diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 3624160a9932d..13057452b4703 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -3175,25 +3175,32 @@ For: ts.Close() } -// Tests that a pipelined request causes the first request's Handler's CloseNotify -// channel to fire. Previously it deadlocked. +// Tests that a pipelined request does not cause the first request's +// Handler's CloseNotify channel to fire. // -// Issue 13165 +// Issue 13165 (where it used to deadlock), but behavior changed in Issue 23921. func TestCloseNotifierPipelined(t *testing.T) { + setParallel(t) defer afterTest(t) gotReq := make(chan bool, 2) sawClose := make(chan bool, 2) ts := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, req *Request) { gotReq <- true cc := rw.(CloseNotifier).CloseNotify() - <-cc + select { + case <-cc: + t.Error("unexpected CloseNotify") + case <-time.After(100 * time.Millisecond): + } sawClose <- true })) + defer ts.Close() conn, err := net.Dial("tcp", ts.Listener.Addr().String()) if err != nil { t.Fatalf("error dialing: %v", err) } diec := make(chan bool, 1) + defer close(diec) go func() { const req = "GET / HTTP/1.1\r\nConnection: keep-alive\r\nHost: foo\r\n\r\n" _, err = io.WriteString(conn, req+req) // two requests @@ -3206,27 +3213,23 @@ func TestCloseNotifierPipelined(t *testing.T) { }() reqs := 0 closes := 0 -For: for { select { case <-gotReq: reqs++ if reqs > 2 { t.Fatal("too many requests") - } else if reqs > 1 { - diec <- true } case <-sawClose: closes++ if closes > 1 { - break For + return } case <-time.After(5 * time.Second): ts.CloseClientConnections() t.Fatal("timeout") } } - ts.Close() } func TestCloseNotifierChanLeak(t *testing.T) { @@ -5796,31 +5799,23 @@ func TestServerHijackGetsBackgroundByte(t *testing.T) { // Tell the client to send more data after the GET request. inHandler <- true - // Wait until the HTTP server sees the extra data - // after the GET request. The HTTP server fires the - // close notifier here, assuming it's a pipelined - // request, as documented. - select { - case <-w.(CloseNotifier).CloseNotify(): - case <-time.After(5 * time.Second): - t.Error("timeout") - return - } - conn, buf, err := w.(Hijacker).Hijack() if err != nil { t.Error(err) return } defer conn.Close() - n := buf.Reader.Buffered() - if n != 1 { - t.Errorf("buffered data = %d; want 1", n) - } + peek, err := buf.Reader.Peek(3) if string(peek) != "foo" || err != nil { t.Errorf("Peek = %q, %v; want foo, nil", peek, err) } + + select { + case <-r.Context().Done(): + t.Error("context unexpectedly canceled") + default: + } })) defer ts.Close() @@ -5861,17 +5856,6 @@ func TestServerHijackGetsBackgroundByte_big(t *testing.T) { ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { defer close(done) - // Wait until the HTTP server sees the extra data - // after the GET request. The HTTP server fires the - // close notifier here, assuming it's a pipelined - // request, as documented. - select { - case <-w.(CloseNotifier).CloseNotify(): - case <-time.After(5 * time.Second): - t.Error("timeout") - return - } - conn, buf, err := w.(Hijacker).Hijack() if err != nil { t.Error(err) diff --git a/src/net/http/server.go b/src/net/http/server.go index f9237d7d71f22..ce785a391682f 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -203,6 +203,9 @@ type Hijacker interface { // // This mechanism can be used to cancel long operations on the server // if the client has disconnected before the response is ready. +// +// Deprecated: the CloseNotifier interface predates Go's context package. +// New code should use Request.Context instead. type CloseNotifier interface { // CloseNotify returns a channel that receives at most a // single value (true) when the client connection has gone @@ -674,10 +677,28 @@ func (cr *connReader) backgroundRead() { cr.lock() if n == 1 { cr.hasByte = true - // We were at EOF already (since we wouldn't be in a - // background read otherwise), so this is a pipelined - // HTTP request. - cr.closeNotifyFromPipelinedRequest() + // We were past the end of the previous request's body already + // (since we wouldn't be in a background read otherwise), so + // this is a pipelined HTTP request. Prior to Go 1.11 we used to + // send on the CloseNotify channel and cancel the context here, + // but the behavior was documented as only "may", and we only + // did that because that's how CloseNotify accidentally behaved + // in very early Go releases prior to context support. Once we + // added context support, people used a Handler's + // Request.Context() and passed it along. Having that context + // cancel on pipelined HTTP requests caused problems. + // Fortunately, almost nothing uses HTTP/1.x pipelining. + // Unfortunately, apt-get does, or sometimes does. + // New Go 1.11 behavior: don't fire CloseNotify or cancel + // contexts on pipelined requests. Shouldn't affect people, but + // fixes cases like Issue 23921. This does mean that a client + // closing their TCP connection after sending a pipelined + // request won't cancel the context, but we'll catch that on any + // write failure (in checkConnErrorWriter.Write). + // If the server never writes, yes, there are still contrived + // server & client behaviors where this fails to ever cancel the + // context, but that's kinda why HTTP/1.x pipelining died + // anyway. } if ne, ok := err.(net.Error); ok && cr.aborted && ne.Timeout() { // Ignore this error. It's the expected error from @@ -724,19 +745,6 @@ func (cr *connReader) handleReadError(_ error) { cr.closeNotify() } -// closeNotifyFromPipelinedRequest simply calls closeNotify. -// -// This method wrapper is here for documentation. The callers are the -// cases where we send on the closenotify channel because of a -// pipelined HTTP request, per the previous Go behavior and -// documentation (that this "MAY" happen). -// -// TODO: consider changing this behavior and making context -// cancelation and closenotify work the same. -func (cr *connReader) closeNotifyFromPipelinedRequest() { - cr.closeNotify() -} - // may be called from multiple goroutines. func (cr *connReader) closeNotify() { res, _ := cr.conn.curReq.Load().(*response) @@ -1834,9 +1842,6 @@ func (c *conn) serve(ctx context.Context) { if requestBodyRemains(req.Body) { registerOnHitEOF(req.Body, w.conn.r.startBackgroundRead) } else { - if w.conn.bufr.Buffered() > 0 { - w.conn.r.closeNotifyFromPipelinedRequest() - } w.conn.r.startBackgroundRead() }