Document Conflict between gorilla/context & Go 1.7's http.Request.WithContext()

[elithrar]

RE: gorilla/mux#182

OK, so this happens because r.WithContext() in Go 1.7, which gorilla/mux uses in Go 1.7, creates a shallow copy of the request: https://github.com/golang/go/blob/master/src/net/http/request.go#L288-L298

This shallow copy, as you'd expect, changes the address of the request in gorilla/context's map[*http.Request]map..., effectively islanding any context values.

This is a shortcoming in gorilla/context: any library, mux included, will break it come Go 1.7 and the http.Request.WithContext() method.`

I'm not sure if there's a clean way to fix this: one option would be to call context.GetAll to copy all values from the old request before we call WithContext, and then save them back into the (shallow) copy, but if you have private types as keys (which is recommended), then we can't reliably achieve that. It would also add context back as a dependency to mux.

• Add a warning to the README
• Document public API so that godoc users also notice the warning when using getters/setters.

cc/ @kisielk

[jordan-wright]

Hi there!

I have a project that uses gorilla/context to store context info on a request. Now that Go 1.7 is out, things aren't working anymore - it's related to this issue.

What's the recommended way to keep context info that's backwards compatible? Is it something like this?

Any help is greatly appreciated - I'm a big fan of the gorilla suite.

[elithrar]

@jordan-wright Yep, use 1.7's context in its entirety. Mixing and matching that and any kind of request map (like gorilla/context) will break things.

[jordan-wright]

Gotcha. Thanks for the quick reply! The issue that I foresee is that the context package was under a different path pre 1.7, and backwards compatibility is important.

You're saying that it's probably best to basically create an abstracted package (like the one I linked to used by mux) that will use the right context package depending on the Go version using build flags?

[elithrar]

@jordan-wright Yep. There really isn't a better way. mux could have kept using gorilla/context in 1.7, but any user who uses (directly, or via a lib) the new http.Request.WithContext() function would break things.

[jordan-wright]

mux could have kept using gorilla/context in 1.7

So I guess that's the part that's throwing me for a loop. I thought gorilla/context was just broken in 1.7.

To give a concrete (simplified) example, my setup is something like this - I've tried to slim it down as much as possible:

import (
    ctx "github.com/gorilla/context"
    "github.com/gorilla/mux"
    "github.com/gorilla/sessions"
    "github.com/gorilla/securecookie"
)

var Store = sessions.NewCookieStore(
    []byte(securecookie.GenerateRandomKey(64)),
    []byte(securecookie.GenerateRandomKey(32)))

router := mux.NewRouter()
router.HandleFunc("/login", Login)

func Login(w http.ResponseWriter, r *http.Request) {
    // The session SET'ing is actually handled in middleware but
    // I wanted to keep things simple
    session, _ := Store.Get(r, "gophish")
    ctx.Set(r, "session", session)
    session := ctx.Get(r, "session").(*sessions.Session)
    // do login stuff
}

So I'm using mux, sessions, and context. The issue I'm running into strictly with Go 1.7 when hitting /login on the line that ctx.Get's the session and type converts is this:

http: panic: interface conversion: interface {} is nil, not *sessions.Session

I'm not using http.Request.WithContext() anywhere... Is the issue that I'm using mux which now uses the new stdlib context package or do you think the issue lies somewhere else?

[elithrar]

mux uses the new stdlib context, hence the issue. Other libraries reported
issues with mux (before it changed), so mux was updated.

Any mixture of any packages that mix the two causes the issue.
gorilla/context is effectively deprecated from Go 1.7 onwards.
On Sun, Sep 11, 2016 at 5:17 PM Jordan Wright notifications@github.com
wrote:

mux could have kept using gorilla/context in 1.7

So I guess that's the part that's throwing me for a loop. I thought
gorilla/context was just broken in 1.7.

To give a concrete (simplified) example, my setup is something like this -
I've tried to slim it down as much as possible:

import (
ctx "github.com/gorilla/context"
"github.com/gorilla/mux"
"github.com/gorilla/sessions"
"github.com/gorilla/securecookie"
)

var Store = sessions.NewCookieStore(
[]byte(securecookie.GenerateRandomKey(64)),
[]byte(securecookie.GenerateRandomKey(32)))

router := mux.NewRouter()
router.HandleFunc("/login", Login)

func Login(w http.ResponseWriter, r _http.Request) {
// The session SET'ing is actually handled in middleware but
// I wanted to keep things simple
session, _ := Store.Get(r, "gophish")
ctx.Set(r, "session", session)
session := ctx.Get(r, "session").(_sessions.Session)
// do login stuff
}

So I'm using mux, sessions, and context. The issue I'm running into
strictly with Go 1.7 when hitting /login on the line that ctx.Get's the
session and type converts is this:

http: panic: interface conversion: interface {} is nil, not *sessions.Session

I'm not using http.Request.WithContext() anywhere... Is the issue that
I'm using mux which now uses the new stdlib context package or do you think
the issue lies somewhere else?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#32 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABIcNnRp1C0Qlmb1yBe3PwKR9iWcESLks5qpJokgaJpZM4Jjt1d
.

[jordan-wright]

Roger that. Thanks for the quick responses, and for all your great work on the Gorilla suite!

[codeskyblue]

@elithrar But it looks gorilla/context is easy to use and understand then the http.Request.WithContext()
Really hope this lib still works.

[elithrar]

@codeskyblue It "works", but if you - or any library you import - uses the new WithContext method on the request, it will break gorilla/context (thus, it doesn't work in practice, and can't be reasonably fixed).

[codeskyblue]

Finally, I modified context, replace *http.Request to *url.URL to make it work after go1.7
master...codeskyblue:master

[Yashiroo]

I'm quite confused. I just migrated from go 1.5 to 1.7 and spent 2 days trying to figure out the issue and finally concluded it was a gorilla/context issue.
Is there any plan to update this issue?

[elithrar]

Where would it be helpful to document (beyond where we already do) this? What would have shortcut the 2 days?

[drichelson]

I just switched to golang 1.7
Using current gorilla/mux and gorilla/context packages (pulled today 5/14/2017)
Previously I had been using the mux route names in an alice middleware via mux.CurrentRoute().
As noted here this is now broken, but it's not clear how I can have a similar functionality with gorilla/mux. The middleware records stats (timings, response codes) for each route by name, so it needs to wrap the request. Any ideas?

[elithrar]

@drichelson - any use of a global request map with Go 1.7's Request.WithContext() will fail.

You should:

a) make sure you are using the latest gorilla/mux
b) update any middleware to stop using gorilla/context and instead use net/http's Request.Context() and WithContext().

gorilla/context can't be changed to use the new context as it would break the API, and be superfluous given that the standard lib now provides a request context.

[drichelson]

@elithrar Thanks for the quick response!
a) I am on the latest master branch of gorilla/mux pulled today (5/14/2017)
b) Nothing is using gorilla/context anymore. I haven't been able to figure out how to get the route name from the http.Request.Context() value.. it doesn't seem to be there. Does mux add the route name to the http.Request's context now? If not, how might I get the route name and add it to the context?

[elithrar]

@drichelson The route name - what (specifically) are you after - the value of route.GetName()? Variables are available in mux.Vars(r), but then name of the route is not propagated through the request context, as it would add a lot of garbage per-request for what is not immediately useful to most users.

If you need to access the name of the route, you should pass it in via middleware.

[drichelson]

@elithrar Yes! I'm after route.GetName(). How would I go about this? It's not clear how the route is made available from within a request.

[elithrar]

(it's not) You should inject it into middleware, or re-think how you need to use the name *inside* a request. Can you explain more about your intent/show an example? Will help me understand how we can achieve this.

…

On Sun, May 14, 2017 at 8:26 PM Dan Richelson ***@***.***> wrote: @elithrar <https://github.com/elithrar> Yes! I'm after route.GetName(). How would I go about this? It's not clear how the route is made available from within a request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcFW8OdNkz_fFHzbCpcCD5KOkvSydks5r58X9gaJpZM4Jjt1d> .

[drichelson]

Here's the way things worked for us before go 1.7: (I've simplified things as much as possible)

1. Many routes with various urls ending up at the same resource and many path variables, so simply inferring a route/resource based on the URL path is tricky and brittle.
2. In router creation:

r := mux.NewRouter()
r..KeepContext = true

3. In Middleware:

	return func(h http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
			route := mux.CurrentRoute(req)
			defer context.Clear(req)
			start := time.Now()
			h.ServeHTTP(mw, req)
			requestDuration := time.Since(start)
			// send requestDuration etc to metrics backend with route.GetName() as the unique route key
		})
	}

I have lots of existing metrics and dashboards based on route.GetName(). Any ideas for how to get it back?

Are you able to show on example of what you mean by "inject it into the middleware"? (this middleware is used across all routes)

[elithrar]

That should work just as before:

package main

import (
	"fmt"
	"log"
	"net/http"

	"github.com/gorilla/mux"
)

func main() {
	r := mux.NewRouter()
	r.Handle("/hello", RouteMiddleware(http.HandlerFunc(HelloHandler))).Name("helloRoute")

	log.Fatal(http.ListenAndServe("localhost:8000", r))
}

func RouteMiddleware(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		route := mux.CurrentRoute(r)
		log.Printf("current route: %s", route.GetName())

	})
}

func HelloHandler(w http.ResponseWriter, r *http.Request) {
	fmt.Fprintln(w, "hello")
}

~/Desktop go run mux.go
~/Desktop curl localhost:8000/hello
2017/05/14 21:44:47 current route: helloRoute

[drichelson]

Thanks @elithrar! This is helpful. It looks like the issue is how we were setting up the middleware:

router := mux.NewRouter()
	chain := alice.New(
		func (h http.Handler) http.Handler {
			// middleware
			return h
		},
	).Then(router)

http.Handle("/", chain)
err := http.ListenAndServe("8000", nil)

[drichelson]

@elithrar Your example works well, but in an app with hundreds of routes, the alice middleware
chaining makes our code a lot cleaner.
To use your example we'd have to wrap many many lines of code looking something like this:

r.Handle("/hello", RouteMiddleware(http.HandlerFunc(HelloHandler))).Name("helloRoute")

We also have several subrouters. Are you aware of a way to add a Middleware to all routes served by the root router? Alice does this for us nicely, but the mux.CurrentRoute() functionality no longer works in alice middleware in go 1.7.

[elithrar]

You can just wrap mux.Router itself - http.ListenAndServe(":8000", middleware(router))

…

On Tue, May 16, 2017 at 1:32 PM Dan Richelson ***@***.***> wrote: @elithrar <https://github.com/elithrar> Your example works well, but in an app with hundreds of routes, the alice middleware chaining makes our code a lot cleaner. To use your example we'd have to wrap many many lines of code looking something like this: r.Handle("/hello", RouteMiddleware(http.HandlerFunc(HelloHandler))).Name("helloRoute") We also have several subrouters. Are you aware of a way to add a Middleware to *all* routes served by the root router? Alice does this for us nicely, but the mux.CurrentRoute() functionality no longer works in alice middleware in go 1.7. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcPc4GXjSJSZI_OM-jH2B6m8ayfH8ks5r6gfbgaJpZM4Jjt1d> .