mux.Vars breaks with Go 1.7

[ejholmes]

Go 1.7 adds native support for context.Context by adding Context and WithContext methods to http.Request.

The expectation is that, if you want to add anything to the context of the request, you use http.Request.WithContext, which returns a shallow copy of the request that you would pass down. Unfortunately, this seems break the way gorilla mux stores vars.

Here's a simple Go program that demonstrates the problem:

package main

import (
    "context"
    "fmt"
    "net/http"
    "net/http/httptest"
    "time"

    "github.com/gorilla/mux"
)

func withTimeout(h http.Handler, timeout time.Duration) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        ctx, cancel := context.WithTimeout(r.Context(), timeout)
        defer cancel()

        r = r.WithContext(ctx)
        h.ServeHTTP(w, r)
    })
}

func ping(w http.ResponseWriter, r *http.Request) {
    fmt.Println(mux.Vars(r))
}

func main() {
    r := mux.NewRouter()
    r.Handle("/broken/{var}", withTimeout(http.HandlerFunc(ping), time.Second))
    r.Handle("/ok/{var}", http.HandlerFunc(ping))

    resp := httptest.NewRecorder()
    req, _ := http.NewRequest("GET", "/ok/foo", nil)
    r.ServeHTTP(resp, req)
    // Prints:
    // map[var:foo]

    resp = httptest.NewRecorder()
    req, _ = http.NewRequest("GET", "/broken/foo", nil)
    r.ServeHTTP(resp, req)
    // Prints:
    // map[]
}

Once Go 1.7 is out, basically anything that uses middleware to set context values, cancellations or deadlines is going to break mux.

It would be ideal of the vars were actually stored in the context.Context, which may be possible without breaking the API?

[elithrar]

Thanks for reporting this—it does makes sense. The new *http.Request is a new pointer and is therefore not the same map key as the original request. I (indirectly) addressed this for gorilla/csrf - gorilla/csrf#35 - by using build tags to use http.Request.Context() in >= Go 1.7.

We can do this without breaking the public API as the only uses of context.Set in setVars and setCurrentRoute:

mux/mux.go

Lines 346 to 356 in bd09be0

	func setVars(r *http.Request, val interface{}) {
	if val != nil {
	context.Set(r, varsKey, val)
	}
	}
	func setCurrentRoute(r *http.Request, val interface{}) {
	if val != nil {
	context.Set(r, routeKey, val)
	}
	}

- these are only called in router.ServeHTTP so we can safely modify these to return the copied *http.Request.

You're welcome to submit a PR (with tests) and/or I'll have something done by early next week. You can pretty much lift the build-tag based implementation out of gorilla/csrf: https://github.com/gorilla/csrf/blob/master/context_legacy.go

PS: My bigger problem is gorilla/sessions which does require breaking the API to fix it as a couple of public functions will need to return a *http.Request.

[ejholmes]

Thanks for the tips @elithrar. I just opened #169 that adds support for this using build tags.