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
}()
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) {
// 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()
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)
//
// 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
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
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)
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()
}