Repository navigation
Go 1.7: http.Request.Context breaks gorilla/context usage #80
Description
Activity
FWIW, I'm currently using @shawnps's branch while awaiting this merge and will report any issues I find.
We inadvertently ran into this issue when we upgraded to Go 1.7.3 recently. The main effect was that our heap used up all of the available memory and our app crashed. It would probably be a good idea to add a warning note to the main README file.
The diagram below is the in-use space after running a 30 second benchmark against one of the routes/handlers in our app that stores various session variables in a cookie.
cc @jawnsy
Thanks for the hint, A workaround is to save the session in the
request.context()manually and still clean up the gorialla context at the end of the request.I'm confused by @jbrook's findings above, and as to whether it's safe to use this package with Go 1.8.
If one follows the advice of wrapping an
http.Handlerwithcontext.ClearHandler, are we safe from accumulating thesesessions.Registryinstances in memory?The
context.ClearHandleruses the request to clean the related data and it might not actually work in some cases and leak some data (depends on any request copy that might be created). But there is a method in the context package that can help:// Purge removes request data stored for longer than maxAge, in seconds. // It returns the amount of requests removed. // // If maxAge <= 0, all request data is removed. // // This is only used for sanity check: in case context cleaning was not // properly set some request data can be kept forever, consuming an increasing // amount of memory. In case this is detected, Purge() must be called // periodically until the problem is fixed. func Purge(maxAge int) int {Just create a
contextPurgeMiddlewarethat callscontext.Purge(-1)after each request. Additionally you have to make sure that you do not rely on anygorilla.contextsince this middleware will wipe contexts of all requests. You must only use the go context than.By my reading of
context.Purge, especially when called with a nonpositive argument, it deletes all the data associated with allhttp.Requests for which we've stored a session. That means thatsessions.(* CookieStore).Getcreatessession.Registrys, but every time we finish with a given request, we remove all of these registries, some of which could be in use in requests we're still processing concurrently. We are then effectively creating caches that we throw away indiscriminately, possibly before we can ever make use of them.Wouldn't the more sound advice be to just avoid all use of
sessions.GetRegistry? It turns out we can't, if ever intend to callsessions.Save. We have to skirt that function and use, say,sessions.(*CookieStore).Savedirectly, which is far less practical.What a mess. We have an attempted optimization that turned out to be an even bigger problem, as hiding things in global variables so often does, and we're now having trouble reworking it in order to be backward compatible. I have no previous application using this package, so I'm more interested in how to break it to fix it.
The original proposal here by @elithrar sounds reasonable, though I'm not sure yet how the callers of
GetRegistryin which I'm interested—sessions.(* CookieStore).Getandsessions.Save—would have to change too.- The registry could likely be slimmed down if we moved to context.Context internally. It would act as a very thin "shim" for when you access session.Values or call session.Save (which is mostly what it is now). Contexts associated with a http.Request via Request.WithContext will automatically be GC'ed and thus we don't need a "purge" mechanism.…On Fri, Jul 21, 2017 at 7:12 PM Steven E. Harris ***@***.***> wrote: By my reading of context.Purge, especially when called with a nonpositive argument <https://sourcegraph.com/github.com/gorilla/context/-/blob/context.go#L119-123>, it deletes *all* the data associated with *all* http.Requests for which we've stored a session. That means that sessions.(* CookieStore).Get creates session.Registrys, but every time we finish with a given request, we remove *all* of these registries, some of which could be in use in requests we're still processing concurrently. We are then effectively creating caches that we throw away indiscriminately, possibly before we can ever make use of them. Wouldn't the more sound advice be to just avoid all use of sessions.GetRegistry? It turns out we can't, if ever intend to call sessions.Save <https://sourcegraph.com/github.com/gorilla/sessions/-/blob/sessions.go#L189-190>. We have to skirt that function and use, say, sessions.(*CookieStore).Save directly, which is far less practical. What a mess. We have an attempted optimization that turned out to be an even bigger problem, as hiding things in global variables so often does, and we're now having trouble reworking it in order to be backward compatible. I have no previous application using this package, so I'm more interested in how to break it to fix it. The original proposal here by @elithrar <https://github.com/elithrar> sounds reasonable, though I'm not sure yet how the callers of GetRegistry in which I'm interested—sessions.(* CookieStore).Get and sessions.Save—would have to change too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcCHZxevsfN5aDaTCBTLIlyEVR9Ehks5sQVqigaJpZM4IuFUJ> .
I think I see a way to avoid all use of the
Registrytype with the package as is, but doing so requires careful study and diligence. I agree that usinghttp.Request.WithContextis the way to go for caching these things, but that does force callers to deal with copiedhttp.Requestvalues.The way that
http.Request.WithContextworks is an odd retrofit. I'd like to find the design rationale that mandated thathttp.Request's "ctx" field be immutable. I suppose that when one is usinghttp.Requestin the context of a server—as opposed to preparing a request as a client—it makes some sense to treat the request as a fixed thing.This article (I apologize for linking to Medium) shows an example of one "outer" HTTP handler preparing a contribution to a request's context, and passing the result of
http.Request.WithContextdirectly on to the next delegated "inner" handler. That looks natural. This package's attempted use of it is awkward, because there's no handler wrapping or delegation in play that establishes a containing and contained extent for the context change.- To note: refactoring this library and breaking the API is easier on a technical level than working within the current constraints. There is however a significant amount of inertia in existing users who don’t use vendoring tools—this package proceeded them + attracts many who are new to Go. I was hoping “dep” would be v1 by now so we could at least guide users to a proven solution (vendor the old tag) but alas.…On Sat, Jul 22, 2017 at 6:53 AM Steven E. Harris ***@***.***> wrote: I *think* I see a way to avoid all use of the Registry type with the package as is, but doing so requires careful study and diligence. I agree that using http.Request.WithContext is the way to go for caching these things, but that does force callers to deal with copied http.Request values. The way that http.Request.WithContext works is an odd retrofit. I'd like to find the design rationale that mandated that http.Request's "ctx" field be immutable. I suppose that when one is using http.Request in the context of a server—as opposed to preparing a request as a client—it makes some sense to treat the request as a fixed thing. This article ***@***.***/context-has-arrived-per-request-state-in-go-1-7-4d095be83bd8> (I apologize for linking to Medium) shows an example of one "outer" HTTP handler preparing a contribution to a request's context, and passing the result of http.Request.WithContext directly on to the next delegated "inner" handler. That looks natural. This package's attempted use of it is awkward, because there's no handler wrapping or delegation in play that establishes a containing and contained extent for the context change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcDCORtCLuB5h7xon9JTyZrie4ITlks5sQf7XgaJpZM4IuFUJ> .
Over the weekend I wrote this library to help with using gorilla/sessions without touching
session.Registry. It needs documentation and examples, but I hope you can get the gist from the function-level documentation that's there.Hmmm, so as a new-ish Golang programmer looking to use gorilla/sessions for the first time... does this affect the code base I'm working on? (using Go 1.8, and not already using any explicit context package)
The current README doc says:
Important Note: If you aren't using gorilla/mux, you need to wrap your handlers with context.ClearHandler ...Which turns out to mean
gorilla/context(only). 😉 Looking at various issues here and the open PR about memory docs it seems like that conflicts with the Go 1.7+contextpackage.Hmmm, does that doc fragment need updating to say something more like:
Important Note: If you are using gorilla/content, but aren't using gorilla/mux, you need to wrap your handlers with context.ClearHandler ...It's pretty confusing atm. 😉
3 remaining items
k, well I'll explain what's confusing me (as dumb as it might sound), as it might shed some clarity on what others experience. 😄
When I (newbie user) read through the session docs it seems reasonably straightforward up until this bit:
Important Note: If you aren't using gorilla/mux, you need to wrap your handlers with context.ClearHandler or else you will leak memory! An easy way to do this is to wrap the top-level mux when calling http.ListenAndServe: http.ListenAndServe(":8080", context.ClearHandler(http.DefaultServeMux)) The ClearHandler function is provided by the gorilla/context package.With the application I'm developing, it's using Go 1.8 and doesn't use gorilla/mux. So, that warning sounds like something we need to do. Leaking memory obviously being bad. Thus:
- Immediate added "context" to our imports
- Didn't work. After re-reading of the above paragraph it mentions a
gorilla/contextpackage. So, that must be the right one to use.
- Look at the gorilla/context page. That one has this warning instead:
Note: gorilla/context, having been born well before context.Context existed, does not play well with the shallow copying of the request that http.Request.WithContext (added to net/http Go 1.7 onwards) performs. You should either use just gorilla/context, or moving forward, the new http.Request.Context().Which makes it sound pretty deprecated. Our code base doesn't have
contextof any sort in any of our imports sections though, so now I'm at a loss which way to go. eg packagesessionssays to importcontext, but packagecontextsounds like it's deprecated- Time to ask.
You've helpfully replied:
If you are using Go 1.8 only and not importing gorilla/context (which includes packages you are using), gorilla/mux defaults to the Go 1.7 http.Request.Context implementation.... but that doesn't really clarify things (for me). In our (newbie) case, we're not using gorilla/mux. We're wanting to use gorilla/sessions. So, not grokking how the gorilla/mux defaulting there comes into play.
Anyway, I guess what I'm meaning is that there needs to be a clear statement about this. The current one doesn't seem to address things for newbie users (99.9% who will be using Go 1.7+) who aren't yet using any
gorillapackages.Hopefully I'm not muddying things. 😄
Reacted by Qifan Xi, Maksim Kozhukh, jacky.chen, Owais Lone and Chua Chee How- added 4 commits that reference this issue
on Aug 14, 2017 Would it be viable to move forward with v2 changes on an alternate branch in this repo? That would at least allow users of
depand similar tools to move forward by specifically selecting the alternate branch/tag, without affecting non-vendor-tool users.Yes, that's what we plan to do, but that's the easy part. The hard part is doing all of the work to write the code, new tests, ensure the new API has time to bake, etc :)
Reacted by Keith MitchellIs it closed by recent PR #175 ?
- Correct.…On Sat, Mar 2, 2019 at 3:32 AM Wilk ***@***.***> wrote: Is it closed by recent PR #175 <#175> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcNJckK9bhN3R0GYa1XbQ4wkGGgtGks5vSmFngaJpZM4IuFUJ> .
Fine, do you plan to make a minor release with this change or still jump to v2 for this ?
- I’ll cut a new minor release for this shortly.…On Mon, Mar 4, 2019 at 1:37 AM Wilk ***@***.***> wrote: Fine, do you plan to make a minor release with this change or still jump to v2 for this ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcKKWgsG7MP9P1N8ZvaEtwlQuT_ebks5vTOlbgaJpZM4IuFUJ> .
The new
http.Request.Context()in Go 1.7 creates a shallow copy of the original request that requires the caller to save it upstream. This copy has a different address and therefore has a different map key in the context map provided by gorilla/context.In order to fix this in gorilla/sessions we need to make a breaking change for Go 1.7+ users:
context.Setis withinGetRegistry- this will now need to return(*Registry, *http.Request).sessions.GetRegistry(r).Get(store, name). They will need to change their code:That should be about it. This is unavoidable unfortunately, but is (thankfully) a compile-time breaking change that we can document clearly.
Ref: gorilla/mux#168