From e1aa744edf2a84efb397511c67b9b252fcb9231a Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Sat, 8 Apr 2017 11:42:24 +0100 Subject: [PATCH 1/5] Remove global map of contexts, use request.Context() --- route/route.go | 50 ++++++-------------------------------------------- 1 file changed, 6 insertions(+), 44 deletions(-) diff --git a/route/route.go b/route/route.go index 1e5638ed9..bb4688173 100644 --- a/route/route.go +++ b/route/route.go @@ -1,26 +1,12 @@ package route import ( - "fmt" "net/http" - "sync" "github.com/julienschmidt/httprouter" "golang.org/x/net/context" ) -var ( - mtx = sync.RWMutex{} - ctxts = map[*http.Request]context.Context{} -) - -// Context returns the context for the request. -func Context(r *http.Request) context.Context { - mtx.RLock() - defer mtx.RUnlock() - return ctxts[r] -} - type param string // Param returns param p for the context. @@ -33,59 +19,35 @@ func WithParam(ctx context.Context, p, v string) context.Context { return context.WithValue(ctx, param(p), v) } -// ContextFunc returns a new context for a request. -type ContextFunc func(r *http.Request) (context.Context, error) - // Router wraps httprouter.Router and adds support for prefixed sub-routers // and per-request context injections. type Router struct { rtr *httprouter.Router prefix string - ctxFn ContextFunc } // New returns a new Router. -func New(ctxFn ContextFunc) *Router { - if ctxFn == nil { - ctxFn = func(r *http.Request) (context.Context, error) { - return context.Background(), nil - } - } +func New() *Router { return &Router{ - rtr: httprouter.New(), - ctxFn: ctxFn, + rtr: httprouter.New(), } } // WithPrefix returns a router that prefixes all registered routes with prefix. func (r *Router) WithPrefix(prefix string) *Router { - return &Router{rtr: r.rtr, prefix: r.prefix + prefix, ctxFn: r.ctxFn} + return &Router{rtr: r.rtr, prefix: r.prefix + prefix} } // handle turns a HandlerFunc into an httprouter.Handle. func (r *Router) handle(h http.HandlerFunc) httprouter.Handle { return func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { - reqCtx, err := r.ctxFn(req) - if err != nil { - http.Error(w, fmt.Sprintf("Error creating request context: %v", err), http.StatusBadRequest) - return - } - ctx, cancel := context.WithCancel(reqCtx) + ctx, cancel := context.WithCancel(req.Context()) defer cancel() for _, p := range params { ctx = context.WithValue(ctx, param(p.Key), p.Value) } - - mtx.Lock() - ctxts[req] = ctx - mtx.Unlock() - - h(w, req) - - mtx.Lock() - delete(ctxts, req) - mtx.Unlock() + h(w, req.WithContext(ctx)) } } @@ -132,7 +94,7 @@ func FileServe(dir string) http.HandlerFunc { fs := http.FileServer(http.Dir(dir)) return func(w http.ResponseWriter, r *http.Request) { - r.URL.Path = Param(Context(r), "filepath") + r.URL.Path = Param(r.Context(), "filepath") fs.ServeHTTP(w, r) } } From 95b9e2ebce366dd9b6095bcdc7bace4fb3681175 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Sat, 8 Apr 2017 12:13:39 +0100 Subject: [PATCH 2/5] Fix up tests --- route/route_test.go | 45 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/route/route_test.go b/route/route_test.go index e7b1cba33..a9bb20996 100644 --- a/route/route_test.go +++ b/route/route_test.go @@ -1,16 +1,13 @@ package route import ( - "fmt" "net/http" "net/http/httptest" "testing" - - "golang.org/x/net/context" ) func TestRedirect(t *testing.T) { - router := New(nil).WithPrefix("/test/prefix") + router := New().WithPrefix("/test/prefix") w := httptest.NewRecorder() r, err := http.NewRequest("GET", "http://localhost:9090/foo", nil) if err != nil { @@ -29,47 +26,19 @@ func TestRedirect(t *testing.T) { } } -func TestContextFunc(t *testing.T) { - router := New(func(r *http.Request) (context.Context, error) { - return context.WithValue(context.Background(), "testkey", "testvalue"), nil - }) - - router.Get("/test", func(w http.ResponseWriter, r *http.Request) { - want := "testvalue" - got := Context(r).Value("testkey") +func TestContext(t *testing.T) { + router := New() + router.Get("/test/:foo/", func(w http.ResponseWriter, r *http.Request) { + want := "bar" + got := Param(r.Context(), "foo") if want != got { t.Fatalf("Unexpected context value: want %q, got %q", want, got) } }) - r, err := http.NewRequest("GET", "http://localhost:9090/test", nil) + r, err := http.NewRequest("GET", "http://localhost:9090/test/bar/", nil) if err != nil { t.Fatalf("Error building test request: %s", err) } router.ServeHTTP(nil, r) } - -func TestContextFnError(t *testing.T) { - router := New(func(r *http.Request) (context.Context, error) { - return context.Background(), fmt.Errorf("test error") - }) - - router.Get("/test", func(w http.ResponseWriter, r *http.Request) {}) - - r, err := http.NewRequest("GET", "http://localhost:9090/test", nil) - if err != nil { - t.Fatalf("Error building test request: %s", err) - } - w := httptest.NewRecorder() - router.ServeHTTP(w, r) - - if w.Code != http.StatusBadRequest { - t.Fatalf("Unexpected response status: got %q, want %q", w.Code, http.StatusBadRequest) - } - - want := "Error creating request context: test error\n" - got := w.Body.String() - if want != got { - t.Fatalf("Unexpected response body: got %q, want %q", got, want) - } -} From a3f28a11677b517350f3f359174cd29c3b73a7a9 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Sat, 8 Apr 2017 15:43:18 +0100 Subject: [PATCH 3/5] Split implementation for pre-go1.7 and go1.7, such that route package contexts work for both. --- route/context_go16.go | 39 +++++++++++++++++++++++++++++++++++++++ route/context_go17.go | 24 ++++++++++++++++++++++++ route/route.go | 9 ++++++--- route/route_test.go | 2 +- 4 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 route/context_go16.go create mode 100644 route/context_go17.go diff --git a/route/context_go16.go b/route/context_go16.go new file mode 100644 index 000000000..1a5e3ffc5 --- /dev/null +++ b/route/context_go16.go @@ -0,0 +1,39 @@ +// +build !go1.7 + +package route + +import ( + "net/http" + "sync" + + "golang.org/x/net/context" +) + +var ( + mtx = sync.RWMutex{} + ctxts = map[*http.Request]context.Context{} +) + +// Context returns the context for the request. +func Context(r *http.Request) context.Context { + mtx.RLock() + defer mtx.RUnlock() + return ctxts[r] +} + +func newContext(r *http.Request) context.Context { + return context.Background() +} + +func setContext(ctx context.Context, r *http.Request) *http.Request { + mtx.Lock() + defer mtx.Unlock() + ctxts[r] = ctx + return r +} + +func deleteContext(r *http.Request) { + mtx.Lock() + defer mtx.Unlock() + delete(ctxts, r) +} diff --git a/route/context_go17.go b/route/context_go17.go new file mode 100644 index 000000000..47fd65411 --- /dev/null +++ b/route/context_go17.go @@ -0,0 +1,24 @@ +// +build go1.7 + +package route + +import ( + "context" + "net/http" +) + +// Context returns the context for the request. +func Context(r *http.Request) context.Context { + return r.Context() +} + +func newContext(r *http.Request) context.Context { + return r.Context() +} + +func setContext(ctx context.Context, r *http.Request) *http.Request { + return r.WithContext(ctx) +} + +func deleteContext(r *http.Request) { +} diff --git a/route/route.go b/route/route.go index bb4688173..2a04ff245 100644 --- a/route/route.go +++ b/route/route.go @@ -41,13 +41,16 @@ func (r *Router) WithPrefix(prefix string) *Router { // handle turns a HandlerFunc into an httprouter.Handle. func (r *Router) handle(h http.HandlerFunc) httprouter.Handle { return func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { - ctx, cancel := context.WithCancel(req.Context()) + ctx, cancel := context.WithCancel(newContext(req)) defer cancel() for _, p := range params { ctx = context.WithValue(ctx, param(p.Key), p.Value) } - h(w, req.WithContext(ctx)) + + req = setContext(ctx, req) + h(w, req) + deleteContext(req) } } @@ -94,7 +97,7 @@ func FileServe(dir string) http.HandlerFunc { fs := http.FileServer(http.Dir(dir)) return func(w http.ResponseWriter, r *http.Request) { - r.URL.Path = Param(r.Context(), "filepath") + r.URL.Path = Param(Context(r), "filepath") fs.ServeHTTP(w, r) } } diff --git a/route/route_test.go b/route/route_test.go index a9bb20996..d1083955e 100644 --- a/route/route_test.go +++ b/route/route_test.go @@ -30,7 +30,7 @@ func TestContext(t *testing.T) { router := New() router.Get("/test/:foo/", func(w http.ResponseWriter, r *http.Request) { want := "bar" - got := Param(r.Context(), "foo") + got := Param(Context(r), "foo") if want != got { t.Fatalf("Unexpected context value: want %q, got %q", want, got) } From d90f04a61a028a34d33064265ae6b556d155602c Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Wed, 12 Apr 2017 08:47:02 -0400 Subject: [PATCH 4/5] Revert "Split implementation for pre-go1.7 and go1.7, such that route package contexts work for both." This reverts commit a3f28a11677b517350f3f359174cd29c3b73a7a9. --- route/context_go16.go | 39 --------------------------------------- route/context_go17.go | 24 ------------------------ route/route.go | 9 +++------ route/route_test.go | 2 +- 4 files changed, 4 insertions(+), 70 deletions(-) delete mode 100644 route/context_go16.go delete mode 100644 route/context_go17.go diff --git a/route/context_go16.go b/route/context_go16.go deleted file mode 100644 index 1a5e3ffc5..000000000 --- a/route/context_go16.go +++ /dev/null @@ -1,39 +0,0 @@ -// +build !go1.7 - -package route - -import ( - "net/http" - "sync" - - "golang.org/x/net/context" -) - -var ( - mtx = sync.RWMutex{} - ctxts = map[*http.Request]context.Context{} -) - -// Context returns the context for the request. -func Context(r *http.Request) context.Context { - mtx.RLock() - defer mtx.RUnlock() - return ctxts[r] -} - -func newContext(r *http.Request) context.Context { - return context.Background() -} - -func setContext(ctx context.Context, r *http.Request) *http.Request { - mtx.Lock() - defer mtx.Unlock() - ctxts[r] = ctx - return r -} - -func deleteContext(r *http.Request) { - mtx.Lock() - defer mtx.Unlock() - delete(ctxts, r) -} diff --git a/route/context_go17.go b/route/context_go17.go deleted file mode 100644 index 47fd65411..000000000 --- a/route/context_go17.go +++ /dev/null @@ -1,24 +0,0 @@ -// +build go1.7 - -package route - -import ( - "context" - "net/http" -) - -// Context returns the context for the request. -func Context(r *http.Request) context.Context { - return r.Context() -} - -func newContext(r *http.Request) context.Context { - return r.Context() -} - -func setContext(ctx context.Context, r *http.Request) *http.Request { - return r.WithContext(ctx) -} - -func deleteContext(r *http.Request) { -} diff --git a/route/route.go b/route/route.go index 2a04ff245..bb4688173 100644 --- a/route/route.go +++ b/route/route.go @@ -41,16 +41,13 @@ func (r *Router) WithPrefix(prefix string) *Router { // handle turns a HandlerFunc into an httprouter.Handle. func (r *Router) handle(h http.HandlerFunc) httprouter.Handle { return func(w http.ResponseWriter, req *http.Request, params httprouter.Params) { - ctx, cancel := context.WithCancel(newContext(req)) + ctx, cancel := context.WithCancel(req.Context()) defer cancel() for _, p := range params { ctx = context.WithValue(ctx, param(p.Key), p.Value) } - - req = setContext(ctx, req) - h(w, req) - deleteContext(req) + h(w, req.WithContext(ctx)) } } @@ -97,7 +94,7 @@ func FileServe(dir string) http.HandlerFunc { fs := http.FileServer(http.Dir(dir)) return func(w http.ResponseWriter, r *http.Request) { - r.URL.Path = Param(Context(r), "filepath") + r.URL.Path = Param(r.Context(), "filepath") fs.ServeHTTP(w, r) } } diff --git a/route/route_test.go b/route/route_test.go index d1083955e..a9bb20996 100644 --- a/route/route_test.go +++ b/route/route_test.go @@ -30,7 +30,7 @@ func TestContext(t *testing.T) { router := New() router.Get("/test/:foo/", func(w http.ResponseWriter, r *http.Request) { want := "bar" - got := Param(Context(r), "foo") + got := Param(r.Context(), "foo") if want != got { t.Fatalf("Unexpected context value: want %q, got %q", want, got) } From c0a169c23ad25bbfb9d4ef2c1a8c15a25d5c943a Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 17 Apr 2017 10:09:53 -0400 Subject: [PATCH 5/5] Remove go 1.5 & 1.6 from travis. --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 69b2431c8..2fe8e9ad7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,6 +2,5 @@ sudo: false language: go go: - - 1.5.4 - - 1.6.2 + - 1.7.5 - tip