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

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="sessions.GetRegistry"+language%3Ago&type=Code&utf8=✓ 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: https://github.com/gorilla/mux/issues/168

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 15
Comments: 22 (10 by maintainers)

Commits related to this issue

#80 add context_legacy.go and context.go for go 1.7 migration — committed to shawnps/sessions by shawnps 8 years ago
#80 add context_legacy.go and context.go for go 1.7 migration — committed to shawnps/sessions by shawnps 8 years ago
#80 return request in Get methods in order to maintain context — committed to shawnps/sessions by shawnps 8 years ago
#80 add context_legacy.go and context.go for go 1.7 migration — committed to shawnps/sessions by shawnps 8 years ago
#80 add note in README about GetRegistry change in Go 1.7 — committed to shawnps/sessions by shawnps 8 years ago
#80 add context_legacy.go and context.go for go 1.7 migration — committed to VREALITY/sessions by shawnps 8 years ago
#80 return request in Get methods in order to maintain context — committed to VREALITY/sessions by shawnps 8 years ago
#80 add context_legacy.go and context.go for go 1.7 migration — committed to VREALITY/sessions by shawnps 8 years ago
#80 return request in Get methods in order to maintain context — committed to VREALITY/sessions by shawnps 8 years ago

Most upvoted comments

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

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

justinclift on Aug 2, 2017

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 😃

elithrar on Jan 22, 2018