diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 2d129f69d3575e..99e3d44d5a659b 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -18,6 +18,7 @@ import ( "encoding/json" "errors" "fmt" + "internal/nettest" "internal/testenv" "io" "log" @@ -8179,3 +8180,107 @@ func TestServerRequestBodyCloseAfterPartialRead(t *testing.T) { } }) } + +// bespokeTimeoutError is a timeout error that does not wrap +// os.ErrDeadlineExceeded, as returned by non-standard net.Conn +// implementations predating the Go 1.15 convention that deadline +// errors wrap os.ErrDeadlineExceeded. +type bespokeTimeoutError struct{} + +func (bespokeTimeoutError) Error() string { return "i/o timeout" } +func (bespokeTimeoutError) Timeout() bool { return true } +func (bespokeTimeoutError) Temporary() bool { return true } + +// bespokeTimeoutConn wraps a net.Conn, replacing read errors that wrap +// os.ErrDeadlineExceeded with bespokeTimeoutError. It emulates +// non-standard net.Conn implementations such as gVisor's gonet. +type bespokeTimeoutConn struct { + net.Conn +} + +func (c bespokeTimeoutConn) Read(p []byte) (int, error) { + n, err := c.Conn.Read(p) + if errors.Is(err, os.ErrDeadlineExceeded) { + err = bespokeTimeoutError{} + } + return n, err +} + +type bespokeTimeoutListener struct { + net.Listener +} + +func (l bespokeTimeoutListener) Accept() (net.Conn, error) { + c, err := l.Listener.Accept() + if err != nil { + return nil, err + } + return bespokeTimeoutConn{c}, nil +} + +// testServerIdleKeepAlive makes two requests on the same keep-alive +// connection, idling between them (in the bubble's fake time) long +// enough for the server's idle buffer release probe (after +// idleBufsReleaseDelay, 50ms) to time out while the connection sits +// idle. It checks that both requests succeed and that neither handler +// sees an already-canceled request context. It must be called in a +// synctest bubble; wrap adapts the server's listener. +func testServerIdleKeepAlive(t *testing.T, wrap func(net.Listener) net.Listener) { + t.Helper() + ctxErr := make(chan error, 1) + srv := &Server{Handler: HandlerFunc(func(w ResponseWriter, r *Request) { + ctxErr <- r.Context().Err() + })} + li := nettest.NewListener() + srvDone := make(chan struct{}) + go func() { + defer close(srvDone) + srv.Serve(wrap(li)) + }() + defer func() { + srv.Close() + <-srvDone + }() + + conn := li.NewConn() + defer conn.Close() + tc := &http1TestConn{t: t, conn: conn, bufr: bufio.NewReader(conn)} + for i := range 2 { + if i == 1 { + synctest.Sleep(250 * time.Millisecond) + } + tc.writeMessage( + "GET / HTTP/1.1", + "Host: foo", + "", + ) + if res := tc.readResponse(); res.StatusCode != 200 { + t.Errorf("request %d: got status %v; want 200", i, res.Status) + } + if err := <-ctxErr; err != nil { + t.Errorf("request %d: handler's Request.Context().Err() = %v; want nil", i, err) + } + } +} + +// TestServerIdleKeepAliveRequestContext verifies that the idle buffer +// release probe between keep-alive requests does not cancel the +// connection-level context that subsequent requests' contexts derive +// from. +func TestServerIdleKeepAliveRequestContext(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + testServerIdleKeepAlive(t, func(li net.Listener) net.Listener { return li }) + }) +} + +// TestServerIdleKeepAliveNonstandardTimeoutError verifies that an idle +// HTTP/1 keep-alive connection survives the idle buffer release probe +// even when the underlying net.Conn returns a timeout error that does +// not wrap os.ErrDeadlineExceeded. +func TestServerIdleKeepAliveNonstandardTimeoutError(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + testServerIdleKeepAlive(t, func(li net.Listener) net.Listener { + return bespokeTimeoutListener{li} + }) + }) +} diff --git a/src/net/http/server.go b/src/net/http/server.go index f2116101af70c2..5cd28fefe0782a 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -23,7 +23,6 @@ import ( "net/textproto" "net/url" urlpkg "net/url" - "os" "path" "runtime" "slices" @@ -711,6 +710,7 @@ type connReader struct { cond *sync.Cond inRead bool aborted bool // set true before conn.rwc deadline is set to past + probing bool // set true during conn.serve's idle probe read, when a timeout is expected remain int64 // bytes remaining } @@ -837,24 +837,51 @@ func (cr *connReader) abortPendingRead() { cr.rwc.SetReadDeadline(time.Time{}) } +func (cr *connReader) setProbing(v bool) { + cr.lock() + cr.probing = v + cr.unlock() +} + func (cr *connReader) setReadLimit(remain int64) { cr.remain = remain } func (cr *connReader) setInfiniteReadLimit() { cr.remain = maxInt64 } func (cr *connReader) hitReadLimit() bool { return cr.remain <= 0 } +// isNetTimeoutError reports whether err is a net.Error with Timeout() +// == true, such as an error from an expired connection deadline. +// It is used instead of checking errors.Is(err, os.ErrDeadlineExceeded) +// because non-standard net.Conn implementations may return bespoke +// timeout errors that don't wrap os.ErrDeadlineExceeded as net package +// connections have since Go 1.15. +func isNetTimeoutError(err error) bool { + ne, ok := errors.AsType[net.Error](err) + return ok && ne.Timeout() +} + // handleReadErrorLocked is called whenever a Read from the client returns a // non-nil error. // // The provided non-nil err is almost always io.EOF or a "use of // closed network connection". In any case, the error is not // particularly interesting, except perhaps for debugging during -// development. Any error means the connection is dead and we should -// down its context. +// development. Except for an expected timeout during the serve loop's +// idle probe read, any error means the connection is dead and we +// should down its context. // // The caller must hold connReader.mu. -func (cr *connReader) handleReadErrorLocked(_ error) { +func (cr *connReader) handleReadErrorLocked(err error) { if cr.conn == nil { return } + // A timeout during conn.serve's idle probe read means only that the + // connection has gone idle; it is otherwise fine. In particular, + // don't cancel the connection-level context: it is the parent of + // every subsequent request's context on this connection, so + // canceling it would deliver already-canceled contexts to all + // future requests. + if cr.probing && isNetTimeoutError(err) { + return + } cr.conn.cancelCtx() if res := cr.conn.curReq.Load(); res != nil { res.closeNotify() @@ -2230,8 +2257,10 @@ func (c *conn) serve(ctx context.Context) { shortDeadline = idleDeadline } c.rwc.SetReadDeadline(shortDeadline) + c.r.setProbing(true) _, peekErr := c.bufr.Peek(4) - if errors.Is(peekErr, os.ErrDeadlineExceeded) && (idleDeadline.IsZero() || time.Now().Before(idleDeadline)) { + c.r.setProbing(false) + if isNetTimeoutError(peekErr) && (idleDeadline.IsZero() || time.Now().Before(idleDeadline)) { c.rwc.SetReadDeadline(idleDeadline) if c.bufr.Buffered() == 0 && c.bufw.Buffered() == 0 { putBufioReader(c.bufr)