From 11133181fce484fee59479785df6ad5b2531411a Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 18 Aug 2015 15:40:14 -0700 Subject: [PATCH 1/2] Fix CloseNotifier handling and avoid "the ResponseWriter does not implement CloseNotifier" warnings in logs A change in #763 to address review comments caused problems. Originally, instrumentedResponseWriter implemented the CloseNotifier interface, and would panic if it was wrapping something that did not implement that interface. This was split into a separate instrumentedResponseWriterCN type that implements CloseNotifier, so there's a fallback if instrumentedResponseWriter ever needs to wrap something that does not implement this interface. instrumentedResponseWriter's Value method would end up upcasting either type back to instrumentedResponseWriter, which does not implement the interface. In effect, instrumentedResponseWriterCN was never visible to the handler. This fixes the problem by implementing a wrapper Value method for instrumentedResponseWriterCN. Signed-off-by: Aaron Lehmann --- docs/handlers/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/handlers/helpers.go b/docs/handlers/helpers.go index a4f3abcc..5a3c9984 100644 --- a/docs/handlers/helpers.go +++ b/docs/handlers/helpers.go @@ -29,7 +29,7 @@ func copyFullPayload(responseWriter http.ResponseWriter, r *http.Request, destWr if notifier, ok := responseWriter.(http.CloseNotifier); ok { clientClosed = notifier.CloseNotify() } else { - ctxu.GetLogger(context).Warn("the ResponseWriter does not implement CloseNotifier") + ctxu.GetLogger(context).Warnf("the ResponseWriter does not implement CloseNotifier (type: %T)", responseWriter) } // Read in the data, if any. From 142b68aaa2c27215b3fdc29a17ed77112bd415e7 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 19 Aug 2015 11:37:53 -0700 Subject: [PATCH 2/2] Add a unit test which verifies the ResponseWriter endpoints see implements CloseNotifier Signed-off-by: Aaron Lehmann --- docs/handlers/api_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/handlers/api_test.go b/docs/handlers/api_test.go index 99168220..e351cb95 100644 --- a/docs/handlers/api_test.go +++ b/docs/handlers/api_test.go @@ -1460,3 +1460,31 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) { checkResponse(t, "deleting blob from cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) } + +// TestCheckContextNotifier makes sure the API endpoints get a ResponseWriter +// that implements http.ContextNotifier. +func TestCheckContextNotifier(t *testing.T) { + env := newTestEnv(t, false) + + // Register a new endpoint for testing + env.app.router.Handle("/unittest/{name}/", env.app.dispatcher(func(ctx *Context, r *http.Request) http.Handler { + return handlers.MethodHandler{ + "GET": http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if _, ok := w.(http.CloseNotifier); !ok { + t.Fatal("could not cast ResponseWriter to CloseNotifier") + } + w.WriteHeader(200) + }), + } + })) + + resp, err := http.Get(env.server.URL + "/unittest/reponame/") + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + t.Fatalf("wrong status code - expected 200, got %d", resp.StatusCode) + } +}