<p>Like I said, I'm pretty new to go. In fact, I'm pretty new to computer science in general (last semester was my first semester without any intro classes). I wrote this package for managing sessions with web applications, and I'm curious what you guys think. I know for a fact there's a lot of things I could optimize and make better, and lot of things that are absolutely necessary that I haven't done (like using mutex's to lock before I change a lot of things). I browse this sub a lot for help, so I pretty much trust you guys. </p>
<p>Link is here: <a href="https://github.com/maudinski/sesh">https://github.com/maudinski/sesh</a>
Its about 200 lines of code without comments. It is pretty commented, and I think the README is worth a read if you choose to dive in. I appreciate in advance any advice you guys can give.</p>
<p>Edit: Thanks <a href="/u/BlueDragonX">u/BlueDragonX</a> , and really all of you, just updated the package a little more. Decreased the API surface by half, and also added some functionality for much safer concurrent access. Still working on the documentation, but even that is a little better. I still have a lot of work to do</p>
<hr/>**评论：**<br/><br/>cavaliercoder: <pre><p>I'll take a closer look shortly, but the first thing I notice is that your comments maybe weren't written with godoc in mind. You can see how your code documentation turns out here: <a href="https://godoc.org/github.com/maudinski/sesh">https://godoc.org/github.com/maudinski/sesh</a>.</p>
<p>If it interests you, take a look at the style of the stdlib docs and mimic it where possible.</p></pre>sabi0: <pre><p>You're right about that, I wasn't even aware that was a thing. Thanks in advance</p></pre>BlueDragonX: <pre><p>Without getting too deeply, a few things:</p>
<ul>
<li>The documentation for this package is incomplete. It does not state what it's for, what problem it solves, or how it solves that problem. All of this should be part of the package documentation such that it show up properly in godoc. See <a href="https://blog.golang.org/godoc-documenting-go-code">this</a>.</li>
<li>The Go ecosystem includes a number of useful linting tools. At the least run your code through <code>go fmt</code> and <code>go lint</code>. Those commands will complain about this code.</li>
<li>Avoid advocating for global variables. There is no reason that the session manager needs to be global. It is better to include it as part of any HTTP handler which requires it.</li>
<li>While <code>sesh.NewSessionManager</code> is not a perfect example of stuttering, it's still not idiomatic. Simply <code>sesh.New</code> is sufficient to convey the meaning.</li>
<li>The API of <code>SessionManager</code> would be cleaner if it could read a cookie from an <code>*http.Request</code> and write one to an <code>http.ResponseWriter</code>. You could include the cookie name when initializing the <code>SessionManager</code> so that it doesn't have to be handled by the caller on each request.</li>
<li><code>NewSessionManager</code> should call <code>NewCustomSessionManager</code>, passing in the default value. Those two functions are copies of each other as it is currently written. Be DRY when prudent.</li>
<li><code>defaultChainSize</code> should be a <code>const</code>, not a <code>var</code>.</li>
<li>It looks as if you're managing slice sizes and re-allocating yourself. Don't do this unless you have a compelling reason to do so (you don't in this case). Use the built-in <code>append</code> instead.</li>
<li>While I don't entirely understand what you're trying to accomplish (the documentation is incomplete) it appears you attempt to check if users are tampering with cookies. If that is the case your solution is almost certainly broken. You're going to want to use some flavor of crypto for this (and don't roll your own).</li>
</ul></pre>sabi0: <pre><p>Ok thank you for that, that was rather thorough. I'll keep all those things in mind. If you don't mine one more question though, on your last bullet point, what do you mean you couldn't understand what I was trying to accomplish? As in the whole program? Or just that I was constantly trying to check for cookie tampering within the program? Like I said I'm novice, but the point of the program was to be a package for easy-maintaining of logged-in state. I was certain that was the idea behind sessions, but like I said, I'm novice, so I could be wrong.</p>
<p>Edit: My documentation could definitely be better. The string being passed to VerifySession and such is not the name of the cookie, but whatever unique name the user is using to store that person in their database. A username, or a user ID, or something like that. I'm definitely going to work on my documentation</p></pre>Floooof: <pre><p>You don't want to use the user id as a unique identifier for the session because they're guessable. Your sessions are only as secure as the cookie values are secret. You can use crypto to sign the cookie values, ensuring that the cookies were issued by your service. If you do that, you don't need to keep them all in memory.</p>
<p>This implementation won't survive a restart of your code, and it won't work if you scale it out to multiple servers. Both scenarios fail because the client holds a reference to a slice in one instance of your app.</p></pre>sabi0: <pre><p>Yeah I definitely need to get into the crypto package. Some one else pointed that out as well. I'm starting to do some research in how how to store stuff in to disk and make it efficient for lookup times. My thoughts are store everything, and as sessions become used, store them in cache for a week or so, then delete them from cache until the user seems to be using them again. Thanks for the advice</p></pre>carsncode: <pre><p>Other comments have covered a lot of good points, but one I would point out as a Go developer: I would never use a session manager without plugable storage back-ends. Having only local storage available makes the application stateful and therefor much harder to scale, load balance, and keep available. In this case they aren't even persisted locally to disk - just restarting the process will lose all sessions.</p>
<p>Some code review:
* NewCustomSM should take an options struct; if you're planning on adding more options later, an options struct would let you do so without changing the function signature (a breaking change)
* StartSession insterts the session at a particular location, but sets the new session's internal "spotMarker" to {-1,0} even though it knows the true value
* The slices of slices doesn't make a whole lot of sense - I'm not sure what a "chain" is supposed to represent but it doesn't seem to add any value
* Slices seem like an odd choice in general; just keeping track of "open spots" is a huge portion of the code and highly error-prone and inefficient. A map seems like a far superior option for this use case.
* The cookies have no expiration/lifetime set, so as soon as the browser window is closed, the sessions end. A configurable session lifetime would be a pretty important feature, and will necessitate a function to refresh the expiration of an existing cookie.</p></pre>sabi0: <pre><p>You said a lot of things I've never thought about. I'm not sure how I would start to implement something with a plugable storage back end, but I'll do some research. Thanks man</p>
<p>Edit: I initially wanted to use a map, but a lot of articles I've read said that Go's implementation of maps is not efficient after a certain amount of entries. Even at that, I more or less tried to structure the chains in a similar fashion to go's maps (which as the developers describe, is a slice of "buckets", and each bucket gets multiple values). The only difference is I think after some point in resizing of go's maps, it become a slice of slices of buckets... and so on. Either way, I'll have to consider everything you said. Thanks a lot, I appreciate it </p></pre>carsncode: <pre><p>I think maps remain efficient up to at least the size you're working with but the only way to say for sure, for your use case, would be too spike them both, benchmark them, and compare them.</p></pre>peterbourgon: <pre><blockquote>
<p>a lot of articles I've read said that Go's implementation of maps is not efficient after a certain amount of entries</p>
</blockquote>
<p>What? This is ridiculous, as a general statement. Where did you read this?</p></pre>sabi0: <pre><p><a href="https://github.com/golang/go/issues/3885" rel="nofollow">https://github.com/golang/go/issues/3885</a></p>
<p><a href="https://www.reddit.com/r/golang/comments/3n0lf8/go_maps_dont_appear_to_be_o1/" rel="nofollow">https://www.reddit.com/r/golang/comments/3n0lf8/go_maps_dont_appear_to_be_o1/</a></p>
<p><a href="https://www.goinggo.net/2013/12/macro-view-of-map-internals-in-go.html" rel="nofollow">https://www.goinggo.net/2013/12/macro-view-of-map-internals-in-go.html</a> - check out the "How Maps Grow" section, but the whole article is worth a read</p>
<p>That second link is somewhat controversial, but has people running tests in it, so worth a look. I guess (and this isn't about go's maps, but about hash tables in general) the reason I elected for some more complex data structures using slices is because this way, the cookie can store information about the exact position in the data structure (which is what I'm doing), making it ALWAYS a constant time lookup, as opposed to hash tables, which are at best constant time lookups. Maybe my logic is a little off, but it seems to be working. And obviously, I need to account for things like parsing the cookie value, etc.
I just ran some tests like 5 min ago of simultaneously running 10000 log ins and log outs on this package (StartSessions and EndSessions) and it was instantaneous. Let me know what you think though, I could just only be getting my hands on negative things about maps.</p></pre>
