Go 1.7: http.Request.Context breaks gorilla/context usage

[elithrar]

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:

• Use build tags to provide two context implementations as per gorilla/csrf: https://github.com/gorilla/csrf/blob/master/context_legacy.go
• The only use of context.Set is within GetRegistry - this will now need to return (*Registry, *http.Request).

- func GetRegistry(r *http.Request) *Registry {
+ func GetRegistry(r *http.Request) (*Registry, *http.Request)

• There is a reasonable amount of code in the wild that this will break: https://github.com/search?q=%22sessions.GetRegistry%22+language%3Ago&type=Code&utf8=%E2%9C%93 as many users are calling sessions.GetRegistry(r).Get(store, name). They will need to change their code:

- sessions.GetRegistry(r).Get(s, name)
+ var reg *sessions.Registry
+ reg, r = sessions.GetRegistry(r)
+ sess, err := reg.Get(store, name)

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

[ironcladlou]

FWIW, I'm currently using @shawnps's branch while awaiting this merge and will report any issues I find.

[jbrook]

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.

[ironcladlou]

cc @jawnsy

[niondir]

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.

[seh]

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.Handler with context.ClearHandler, are we safe from accumulating these sessions.Registry instances in memory?

[niondir]

The context.ClearHandler uses 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 contextPurgeMiddleware that calls context.Purge(-1) after each request. Additionally you have to make sure that you do not rely on any gorilla.context since this middleware will wipe contexts of all requests. You must only use the go context than.

[seh]

By my reading of context.Purge, especially when called with a nonpositive argument, 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. 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 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.

[elithrar]

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> .

[seh]

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 (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.

[elithrar]

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> .

[seh]

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.

[justinclift]

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+ context package.

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. 😉

[elithrar]

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.

…

On Wed, Aug 2, 2017 at 7:42 AM Justin Clift ***@***.***> wrote: 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+ context package. 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. 😉 — 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/AABIcLwCza_i7Hm2BWqOC4EmNJKpHqOTks5sUIrdgaJpZM4IuFUJ> .

[justinclift]

Cool. What's the right way to communicate that to new Go developers, so they know if/what they'll need to change? 😄

[elithrar]

To be honest: if I knew I'd have done it already. What isn't clear about the current phrasing, given that it links the gorilla/context package too? (This stuff is dense as well, so it's just hard for new developers to understand - not their fault, we're dealing with tech debt)

…

On Wed, Aug 2, 2017 at 8:45 AM Justin Clift ***@***.***> wrote: Cool. What's the right way to communicate that to new Go developers, so they know if/what they'll need to 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/AABIcHEAAbp0Q5-FGSbGzj3TgO7IJ42Qks5sUJmugaJpZM4IuFUJ> .

[justinclift]

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:

1. Immediate added "context" to our imports

• Didn't work. After re-reading of the above paragraph it mentions a gorilla/context package. So, that must be the right one to use.

2. 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 context of any sort in any of our imports sections though, so now I'm at a loss which way to go. eg package sessions says to import context, but package context sounds like it's deprecated

3. 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 gorilla packages.

Hopefully I'm not muddying things. 😄

[kemitche]

Would it be viable to move forward with v2 changes on an alternate branch in this repo? That would at least allow users of dep and similar tools to move forward by specifically selecting the alternate branch/tag, without affecting non-vendor-tool users.

[elithrar]

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 :)

[flibustenet]

Is it closed by recent PR #175 ?

[elithrar]

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> .

[flibustenet]

Fine, do you plan to make a minor release with this change or still jump to v2 for this ?

[elithrar]

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> .