fix(middleware): bound post-handler work and hijack under the inactivity timeout
Arm bounded read/write deadlines before the handler returns so net/http's final flush and unread-body drain cannot block forever with no server Read/WriteTimeout. Hijack no longer holds the state mutex across I/O, so the timer keeps enforcing until the hijack succeeds. Correct the documented write floor to about 68 B/s and make the early-response upload test assert the request succeeds.
- commit
- 2a5c221460498f1263d88a4601d5ed5860cf38e1
- author
- brice · · 2026-10-16 19:26 UTC
- parent
- 98315bd
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +195 -26
M docs/config/traffic.md +2 -2
| @@ -148,9 +148,9 @@ Values are Go duration strings; malformed, zero, or negative values are rejected | |||
| 148 | 148 | ||
| 149 | 149 | ### Inactivity timeout | |
| 150 | 150 | ||
| 151 | - | Each request has an inactivity timer, armed when the request starts. Progress resets it: a response chunk accepted by the client connection (8 KiB granularity), a flush, or request body bytes read. If `transfer_idle_timeout` passes without progress, Gordon cancels the request context, which aborts the upstream request of a reverse-proxied app, and closes the client connection. Consequences: | |
| 151 | + | Each request has an inactivity timer, armed when the request starts. Progress resets it: a response chunk accepted by the client connection (8 KiB granularity), a flush, or request body bytes read. If `transfer_idle_timeout` passes without progress, Gordon cancels the request context, which aborts the upstream request of a reverse-proxied app, and closes the client connection. When the handler returns, the final response flush and the drain of an unread request body are bounded by the same duration. Consequences: | |
| 152 | 152 | ||
| 153 | - | - a response that streams data at least once per `transfer_idle_timeout` can last arbitrarily long, and a client that keeps reading is never cut (it must accept 8 KiB per period at minimum); | |
| 153 | + | - a response that streams data at least once per `transfer_idle_timeout` can last arbitrarily long, and a client that keeps reading is never cut (it must accept 8 KiB per `transfer_idle_timeout`, about 68 B/s at the 2 minute default); | |
| 154 | 154 | - an upload whose body arrives at least once per `transfer_idle_timeout` can last arbitrarily long; | |
| 155 | 155 | - a backend that stays silent longer than `transfer_idle_timeout` before or between response chunks has its connection cut, so set it above the slowest expected backend response time; | |
| 156 | 156 | - the timer also fires when a handler or backend is silent and no write is pending; | |
| @@ -148,9 +148,9 @@ Values are Go duration strings; malformed, zero, or negative values are rejected | |||||
| 148 | 148 | ||||
| 149 | ### Inactivity timeout | 149 | ### Inactivity timeout | ||
| 150 | 150 | ||||
| 151 | - | Each request has an inactivity timer, armed when the request starts. Progress resets it: a response chunk accepted by the client connection (8 KiB granularity), a flush, or request body bytes read. If `transfer_idle_timeout` passes without progress, Gordon cancels the request context, which aborts the upstream request of a reverse-proxied app, and closes the client connection. Consequences: | 151 | + | Each request has an inactivity timer, armed when the request starts. Progress resets it: a response chunk accepted by the client connection (8 KiB granularity), a flush, or request body bytes read. If `transfer_idle_timeout` passes without progress, Gordon cancels the request context, which aborts the upstream request of a reverse-proxied app, and closes the client connection. When the handler returns, the final response flush and the drain of an unread request body are bounded by the same duration. Consequences: |
| 152 | 152 | ||||
| 153 | - | - a response that streams data at least once per `transfer_idle_timeout` can last arbitrarily long, and a client that keeps reading is never cut (it must accept 8 KiB per period at minimum); | 153 | + | - a response that streams data at least once per `transfer_idle_timeout` can last arbitrarily long, and a client that keeps reading is never cut (it must accept 8 KiB per `transfer_idle_timeout`, about 68 B/s at the 2 minute default); |
| 154 | - an upload whose body arrives at least once per `transfer_idle_timeout` can last arbitrarily long; | 154 | - an upload whose body arrives at least once per `transfer_idle_timeout` can last arbitrarily long; | ||
| 155 | - a backend that stays silent longer than `transfer_idle_timeout` before or between response chunks has its connection cut, so set it above the slowest expected backend response time; | 155 | - a backend that stays silent longer than `transfer_idle_timeout` before or between response chunks has its connection cut, so set it above the slowest expected backend response time; | ||
| 156 | - the timer also fires when a handler or backend is silent and no write is pending; | 156 | - the timer also fires when a handler or backend is silent and no write is pending; | ||
M internal/adapters/in/http/middleware/transfer_idle.go +54 -17
| @@ -20,7 +20,7 @@ var errTransferFinished = errors.New("transfer idle: request already finished") | |||
| 20 | 20 | // accepted, so a client that keeps consuming data is never cut while a client | |
| 21 | 21 | // that stops reading is cut after the idle timeout. The residual granularity is | |
| 22 | 22 | // one chunk: a client must accept idleWriteChunk bytes within one idle period, | |
| 23 | - | // which is a floor of idleWriteChunk/idle bytes per second (about 27 B/s for | |
| 23 | + | // which is a floor of idleWriteChunk/idle bytes per second (about 68 B/s for | |
| 24 | 24 | // the 2 minute default). | |
| 25 | 25 | const idleWriteChunk = 8 * 1024 | |
| 26 | 26 | ||
| @@ -42,6 +42,15 @@ const idleWriteChunk = 8 * 1024 | |||
| 42 | 42 | // Deadlines go through http.ResponseController, so HTTP/1.x and HTTP/2 both | |
| 43 | 43 | // work; Flush, Hijack and Unwrap are preserved. | |
| 44 | 44 | // | |
| 45 | + | // net/http does work after the handler returns that can block on a stalled | |
| 46 | + | // client: the final response flush and the drain of an unread request body. | |
| 47 | + | // With no server Read/WriteTimeout those would hang forever, so finish arms | |
| 48 | + | // one last bounded read and write deadline (idle from now) before the handler | |
| 49 | + | // returns and leaves it in place. net/http clears the write deadline itself | |
| 50 | + | // after the response is finished and replaces the read deadline with the | |
| 51 | + | // keep-alive idle / header deadline before the next request, so nothing leaks | |
| 52 | + | // into the next request. The timer never touches the controller after return. | |
| 53 | + | // | |
| 45 | 54 | // Limits: a handler that ignores the canceled context and never touches the | |
| 46 | 55 | // connection again keeps its HTTP/1 connection until it returns. A non-positive | |
| 47 | 56 | // idle disables the middleware. | |
| @@ -52,13 +61,14 @@ func TransferIdleTimeout(idle time.Duration) func(http.Handler) http.Handler { | |||
| 52 | 61 | } | |
| 53 | 62 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 54 | 63 | ctx, cancel := context.WithCancel(r.Context()) | |
| 55 | - | state := newIdleState(idle, http.NewResponseController(w), cancel) | |
| 64 | + | hasBody := r.Body != nil && r.Body != http.NoBody | |
| 65 | + | state := newIdleState(idle, http.NewResponseController(w), cancel, hasBody) | |
| 56 | 66 | // finish marks the state done under the mutex before this | |
| 57 | 67 | // function returns, so nothing touches the controller afterwards. | |
| 58 | 68 | defer state.finish() | |
| 59 | 69 | ||
| 60 | 70 | req := r.WithContext(ctx) | |
| 61 | - | if r.Body != nil && r.Body != http.NoBody { | |
| 71 | + | if hasBody { | |
| 62 | 72 | req.Body = &idleBody{ReadCloser: r.Body, state: state} | |
| 63 | 73 | } | |
| 64 | 74 | next.ServeHTTP(&idleResponseWriter{ResponseWriter: w, state: state}, req) | |
| @@ -73,14 +83,18 @@ type idleState struct { | |||
| 73 | 83 | rc *http.ResponseController | |
| 74 | 84 | cancel context.CancelFunc | |
| 75 | 85 | ||
| 76 | - | mu sync.Mutex | |
| 77 | - | timer *time.Timer | |
| 78 | - | last time.Time | |
| 79 | - | done bool // handler returned or connection hijacked | |
| 86 | + | hasBody bool // request has a body that net/http may still have to drain | |
| 87 | + | ||
| 88 | + | mu sync.Mutex | |
| 89 | + | timer *time.Timer | |
| 90 | + | last time.Time | |
| 91 | + | done bool // handler returned or connection hijacked | |
| 92 | + | expired bool // the timer fired and expired the connection deadlines | |
| 93 | + | hijacked bool | |
| 80 | 94 | } | |
| 81 | 95 | ||
| 82 | - | func newIdleState(idle time.Duration, rc *http.ResponseController, cancel context.CancelFunc) *idleState { | |
| 83 | - | s := &idleState{idle: idle, rc: rc, cancel: cancel, last: time.Now()} | |
| 96 | + | func newIdleState(idle time.Duration, rc *http.ResponseController, cancel context.CancelFunc, hasBody bool) *idleState { | |
| 97 | + | s := &idleState{idle: idle, rc: rc, cancel: cancel, last: time.Now(), hasBody: hasBody} | |
| 84 | 98 | s.mu.Lock() | |
| 85 | 99 | s.timer = time.AfterFunc(idle, s.onTimer) | |
| 86 | 100 | s.mu.Unlock() | |
| @@ -116,6 +130,7 @@ func (s *idleState) onTimer() { | |||
| 116 | 130 | s.timer.Reset(remaining) | |
| 117 | 131 | return | |
| 118 | 132 | } | |
| 133 | + | s.expired = true | |
| 119 | 134 | s.cancel() | |
| 120 | 135 | // Held under mu so finish() cannot complete while these run. | |
| 121 | 136 | now := time.Now() | |
| @@ -124,11 +139,21 @@ func (s *idleState) onTimer() { | |||
| 124 | 139 | } | |
| 125 | 140 | ||
| 126 | 141 | // finish ends the request lifetime. After it returns, no code path calls the | |
| 127 | - | // controller. It is safe to call more than once. | |
| 142 | + | // controller. It is safe to call more than once. Unless the connection was | |
| 143 | + | // hijacked or already expired, it first arms bounded deadlines so net/http's | |
| 144 | + | // post-handler work (final flush, unread body drain) cannot block forever. | |
| 128 | 145 | func (s *idleState) finish() { | |
| 129 | 146 | s.mu.Lock() | |
| 147 | + | wasDone := s.done | |
| 130 | 148 | s.done = true | |
| 131 | 149 | s.timer.Stop() | |
| 150 | + | if !wasDone && !s.expired && !s.hijacked { | |
| 151 | + | deadline := time.Now().Add(s.idle) | |
| 152 | + | _ = s.rc.SetWriteDeadline(deadline) | |
| 153 | + | if s.hasBody { | |
| 154 | + | _ = s.rc.SetReadDeadline(deadline) | |
| 155 | + | } | |
| 156 | + | } | |
| 132 | 157 | s.mu.Unlock() | |
| 133 | 158 | s.cancel() | |
| 134 | 159 | } | |
| @@ -146,18 +171,30 @@ func (s *idleState) flush() error { | |||
| 146 | 171 | return s.rc.Flush() | |
| 147 | 172 | } | |
| 148 | 173 | ||
| 174 | + | // hijack hijacks the connection without holding the mutex: HTTP/1 Hijack may | |
| 175 | + | // flush pending headers and block on a client that does not read, and the timer | |
| 176 | + | // needs the mutex to expire the deadlines that unblock it. Enforcement stays | |
| 177 | + | // active until the hijack succeeds; only then is the timer stopped. If the | |
| 178 | + | // timer fired meanwhile, the deadlines it set on the connection are cleared so | |
| 179 | + | // the upgraded stream is not cut. | |
| 149 | 180 | func (s *idleState) hijack() (net.Conn, *bufio.ReadWriter, error) { | |
| 150 | - | s.mu.Lock() | |
| 151 | - | defer s.mu.Unlock() | |
| 152 | - | if s.done { | |
| 181 | + | if !s.live() { | |
| 153 | 182 | return nil, nil, errTransferFinished | |
| 154 | 183 | } | |
| 155 | 184 | conn, rw, err := s.rc.Hijack() | |
| 156 | - | if err == nil { | |
| 157 | - | s.done = true | |
| 158 | - | s.timer.Stop() | |
| 185 | + | if err != nil { | |
| 186 | + | return conn, rw, err | |
| 187 | + | } | |
| 188 | + | s.mu.Lock() | |
| 189 | + | s.done = true | |
| 190 | + | s.hijacked = true | |
| 191 | + | s.timer.Stop() | |
| 192 | + | expired := s.expired | |
| 193 | + | s.mu.Unlock() | |
| 194 | + | if expired { | |
| 195 | + | _ = conn.SetDeadline(time.Time{}) | |
| 159 | 196 | } | |
| 160 | - | return conn, rw, err | |
| 197 | + | return conn, rw, nil | |
| 161 | 198 | } | |
| 162 | 199 | ||
| 163 | 200 | // idleResponseWriter reports response progress. | |
| @@ -20,7 +20,7 @@ var errTransferFinished = errors.New("transfer idle: request already finished") | |||||
| 20 | // accepted, so a client that keeps consuming data is never cut while a client | 20 | // accepted, so a client that keeps consuming data is never cut while a client | ||
| 21 | // that stops reading is cut after the idle timeout. The residual granularity is | 21 | // that stops reading is cut after the idle timeout. The residual granularity is | ||
| 22 | // one chunk: a client must accept idleWriteChunk bytes within one idle period, | 22 | // one chunk: a client must accept idleWriteChunk bytes within one idle period, | ||
| 23 | - | // which is a floor of idleWriteChunk/idle bytes per second (about 27 B/s for | 23 | + | // which is a floor of idleWriteChunk/idle bytes per second (about 68 B/s for |
| 24 | // the 2 minute default). | 24 | // the 2 minute default). | ||
| 25 | const idleWriteChunk = 8 * 1024 | 25 | const idleWriteChunk = 8 * 1024 | ||
| 26 | 26 | ||||
| @@ -42,6 +42,15 @@ const idleWriteChunk = 8 * 1024 | |||||
| 42 | // Deadlines go through http.ResponseController, so HTTP/1.x and HTTP/2 both | 42 | // Deadlines go through http.ResponseController, so HTTP/1.x and HTTP/2 both | ||
| 43 | // work; Flush, Hijack and Unwrap are preserved. | 43 | // work; Flush, Hijack and Unwrap are preserved. | ||
| 44 | // | 44 | // | ||
| 45 | + | // net/http does work after the handler returns that can block on a stalled | |||
| 46 | + | // client: the final response flush and the drain of an unread request body. | |||
| 47 | + | // With no server Read/WriteTimeout those would hang forever, so finish arms | |||
| 48 | + | // one last bounded read and write deadline (idle from now) before the handler | |||
| 49 | + | // returns and leaves it in place. net/http clears the write deadline itself | |||
| 50 | + | // after the response is finished and replaces the read deadline with the | |||
| 51 | + | // keep-alive idle / header deadline before the next request, so nothing leaks | |||
| 52 | + | // into the next request. The timer never touches the controller after return. | |||
| 53 | + | // | |||
| 45 | // Limits: a handler that ignores the canceled context and never touches the | 54 | // Limits: a handler that ignores the canceled context and never touches the | ||
| 46 | // connection again keeps its HTTP/1 connection until it returns. A non-positive | 55 | // connection again keeps its HTTP/1 connection until it returns. A non-positive | ||
| 47 | // idle disables the middleware. | 56 | // idle disables the middleware. | ||
| @@ -52,13 +61,14 @@ func TransferIdleTimeout(idle time.Duration) func(http.Handler) http.Handler { | |||||
| 52 | } | 61 | } | ||
| 53 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 62 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 54 | ctx, cancel := context.WithCancel(r.Context()) | 63 | ctx, cancel := context.WithCancel(r.Context()) | ||
| 55 | - | state := newIdleState(idle, http.NewResponseController(w), cancel) | 64 | + | hasBody := r.Body != nil && r.Body != http.NoBody |
| 65 | + | state := newIdleState(idle, http.NewResponseController(w), cancel, hasBody) | |||
| 56 | // finish marks the state done under the mutex before this | 66 | // finish marks the state done under the mutex before this | ||
| 57 | // function returns, so nothing touches the controller afterwards. | 67 | // function returns, so nothing touches the controller afterwards. | ||
| 58 | defer state.finish() | 68 | defer state.finish() | ||
| 59 | 69 | ||||
| 60 | req := r.WithContext(ctx) | 70 | req := r.WithContext(ctx) | ||
| 61 | - | if r.Body != nil && r.Body != http.NoBody { | 71 | + | if hasBody { |
| 62 | req.Body = &idleBody{ReadCloser: r.Body, state: state} | 72 | req.Body = &idleBody{ReadCloser: r.Body, state: state} | ||
| 63 | } | 73 | } | ||
| 64 | next.ServeHTTP(&idleResponseWriter{ResponseWriter: w, state: state}, req) | 74 | next.ServeHTTP(&idleResponseWriter{ResponseWriter: w, state: state}, req) | ||
| @@ -73,14 +83,18 @@ type idleState struct { | |||||
| 73 | rc *http.ResponseController | 83 | rc *http.ResponseController | ||
| 74 | cancel context.CancelFunc | 84 | cancel context.CancelFunc | ||
| 75 | 85 | ||||
| 76 | - | mu sync.Mutex | 86 | + | hasBody bool // request has a body that net/http may still have to drain |
| 77 | - | timer *time.Timer | 87 | + | |
| 78 | - | last time.Time | 88 | + | mu sync.Mutex |
| 79 | - | done bool // handler returned or connection hijacked | 89 | + | timer *time.Timer |
| 90 | + | last time.Time | |||
| 91 | + | done bool // handler returned or connection hijacked | |||
| 92 | + | expired bool // the timer fired and expired the connection deadlines | |||
| 93 | + | hijacked bool | |||
| 80 | } | 94 | } | ||
| 81 | 95 | ||||
| 82 | - | func newIdleState(idle time.Duration, rc *http.ResponseController, cancel context.CancelFunc) *idleState { | 96 | + | func newIdleState(idle time.Duration, rc *http.ResponseController, cancel context.CancelFunc, hasBody bool) *idleState { |
| 83 | - | s := &idleState{idle: idle, rc: rc, cancel: cancel, last: time.Now()} | 97 | + | s := &idleState{idle: idle, rc: rc, cancel: cancel, last: time.Now(), hasBody: hasBody} |
| 84 | s.mu.Lock() | 98 | s.mu.Lock() | ||
| 85 | s.timer = time.AfterFunc(idle, s.onTimer) | 99 | s.timer = time.AfterFunc(idle, s.onTimer) | ||
| 86 | s.mu.Unlock() | 100 | s.mu.Unlock() | ||
| @@ -116,6 +130,7 @@ func (s *idleState) onTimer() { | |||||
| 116 | s.timer.Reset(remaining) | 130 | s.timer.Reset(remaining) | ||
| 117 | return | 131 | return | ||
| 118 | } | 132 | } | ||
| 133 | + | s.expired = true | |||
| 119 | s.cancel() | 134 | s.cancel() | ||
| 120 | // Held under mu so finish() cannot complete while these run. | 135 | // Held under mu so finish() cannot complete while these run. | ||
| 121 | now := time.Now() | 136 | now := time.Now() | ||
| @@ -124,11 +139,21 @@ func (s *idleState) onTimer() { | |||||
| 124 | } | 139 | } | ||
| 125 | 140 | ||||
| 126 | // finish ends the request lifetime. After it returns, no code path calls the | 141 | // finish ends the request lifetime. After it returns, no code path calls the | ||
| 127 | - | // controller. It is safe to call more than once. | 142 | + | // controller. It is safe to call more than once. Unless the connection was |
| 143 | + | // hijacked or already expired, it first arms bounded deadlines so net/http's | |||
| 144 | + | // post-handler work (final flush, unread body drain) cannot block forever. | |||
| 128 | func (s *idleState) finish() { | 145 | func (s *idleState) finish() { | ||
| 129 | s.mu.Lock() | 146 | s.mu.Lock() | ||
| 147 | + | wasDone := s.done | |||
| 130 | s.done = true | 148 | s.done = true | ||
| 131 | s.timer.Stop() | 149 | s.timer.Stop() | ||
| 150 | + | if !wasDone && !s.expired && !s.hijacked { | |||
| 151 | + | deadline := time.Now().Add(s.idle) | |||
| 152 | + | _ = s.rc.SetWriteDeadline(deadline) | |||
| 153 | + | if s.hasBody { | |||
| 154 | + | _ = s.rc.SetReadDeadline(deadline) | |||
| 155 | + | } | |||
| 156 | + | } | |||
| 132 | s.mu.Unlock() | 157 | s.mu.Unlock() | ||
| 133 | s.cancel() | 158 | s.cancel() | ||
| 134 | } | 159 | } | ||
| @@ -146,18 +171,30 @@ func (s *idleState) flush() error { | |||||
| 146 | return s.rc.Flush() | 171 | return s.rc.Flush() | ||
| 147 | } | 172 | } | ||
| 148 | 173 | ||||
| 174 | + | // hijack hijacks the connection without holding the mutex: HTTP/1 Hijack may | |||
| 175 | + | // flush pending headers and block on a client that does not read, and the timer | |||
| 176 | + | // needs the mutex to expire the deadlines that unblock it. Enforcement stays | |||
| 177 | + | // active until the hijack succeeds; only then is the timer stopped. If the | |||
| 178 | + | // timer fired meanwhile, the deadlines it set on the connection are cleared so | |||
| 179 | + | // the upgraded stream is not cut. | |||
| 149 | func (s *idleState) hijack() (net.Conn, *bufio.ReadWriter, error) { | 180 | func (s *idleState) hijack() (net.Conn, *bufio.ReadWriter, error) { | ||
| 150 | - | s.mu.Lock() | 181 | + | if !s.live() { |
| 151 | - | defer s.mu.Unlock() | |||
| 152 | - | if s.done { | |||
| 153 | return nil, nil, errTransferFinished | 182 | return nil, nil, errTransferFinished | ||
| 154 | } | 183 | } | ||
| 155 | conn, rw, err := s.rc.Hijack() | 184 | conn, rw, err := s.rc.Hijack() | ||
| 156 | - | if err == nil { | 185 | + | if err != nil { |
| 157 | - | s.done = true | 186 | + | return conn, rw, err |
| 158 | - | s.timer.Stop() | 187 | + | } |
| 188 | + | s.mu.Lock() | |||
| 189 | + | s.done = true | |||
| 190 | + | s.hijacked = true | |||
| 191 | + | s.timer.Stop() | |||
| 192 | + | expired := s.expired | |||
| 193 | + | s.mu.Unlock() | |||
| 194 | + | if expired { | |||
| 195 | + | _ = conn.SetDeadline(time.Time{}) | |||
| 159 | } | 196 | } | ||
| 160 | - | return conn, rw, err | 197 | + | return conn, rw, nil |
| 161 | } | 198 | } | ||
| 162 | 199 | ||||
| 163 | // idleResponseWriter reports response progress. | 200 | // idleResponseWriter reports response progress. | ||
M internal/adapters/in/http/middleware/transfer_idle_test.go +139 -7
| @@ -452,19 +452,23 @@ func uploadUntilEarlyResponse(t *testing.T, front *httptest.Server) { | |||
| 452 | 452 | _ = pw.Close() | |
| 453 | 453 | }() | |
| 454 | 454 | ||
| 455 | + | defer func() { | |
| 456 | + | _ = pr.CloseWithError(io.ErrClosedPipe) | |
| 457 | + | <-writerDone | |
| 458 | + | }() | |
| 459 | + | ||
| 455 | 460 | req, err := http.NewRequest(http.MethodPost, front.URL, pr) | |
| 456 | 461 | if !assert.NoError(t, err) { | |
| 457 | 462 | return | |
| 458 | 463 | } | |
| 459 | 464 | resp, err := front.Client().Do(req) | |
| 460 | - | if err == nil { | |
| 461 | - | assert.Equal(t, 2, resp.ProtoMajor) | |
| 462 | - | assert.Equal(t, http.StatusRequestEntityTooLarge, resp.StatusCode) | |
| 463 | - | _, _ = io.Copy(io.Discard, resp.Body) | |
| 464 | - | _ = resp.Body.Close() | |
| 465 | + | if !assert.NoError(t, err, "the early 413 must reach the client") { | |
| 466 | + | return | |
| 465 | 467 | } | |
| 466 | - | _ = pr.CloseWithError(io.ErrClosedPipe) | |
| 467 | - | <-writerDone | |
| 468 | + | defer resp.Body.Close() | |
| 469 | + | assert.Equal(t, 2, resp.ProtoMajor) | |
| 470 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, resp.StatusCode) | |
| 471 | + | _, _ = io.Copy(io.Discard, resp.Body) | |
| 468 | 472 | } | |
| 469 | 473 | ||
| 470 | 474 | // TestTransferIdleTimeout_HTTP2BodyUsedAfterHandlerReturned uses the request | |
| @@ -504,3 +508,131 @@ func TestTransferIdleTimeout_HTTP2BodyUsedAfterHandlerReturned(t *testing.T) { | |||
| 504 | 508 | require.NoError(t, body.Close()) | |
| 505 | 509 | require.NoError(t, body.Close()) | |
| 506 | 510 | } | |
| 511 | + | ||
| 512 | + | // trackedPipeServer serves net.Pipe connections and reports when the server | |
| 513 | + | // side closes one, so tests can assert a stalled connection is released. | |
| 514 | + | func trackedPipeServer(t *testing.T, handler http.Handler) (*pipeListener, <-chan struct{}) { | |
| 515 | + | t.Helper() | |
| 516 | + | closed := make(chan struct{}) | |
| 517 | + | var once sync.Once | |
| 518 | + | ln := newPipeListener() | |
| 519 | + | srv := &http.Server{ | |
| 520 | + | Handler: TransferIdleTimeout(testIdle)(handler), | |
| 521 | + | ReadHeaderTimeout: 5 * time.Second, | |
| 522 | + | ConnState: func(_ net.Conn, state http.ConnState) { | |
| 523 | + | if state == http.StateClosed { | |
| 524 | + | once.Do(func() { close(closed) }) | |
| 525 | + | } | |
| 526 | + | }, | |
| 527 | + | } | |
| 528 | + | go func() { _ = srv.Serve(ln) }() | |
| 529 | + | t.Cleanup(func() { _ = srv.Close() }) | |
| 530 | + | return ln, closed | |
| 531 | + | } | |
| 532 | + | ||
| 533 | + | func requireClosed(t *testing.T, closed <-chan struct{}, what string) { | |
| 534 | + | t.Helper() | |
| 535 | + | select { | |
| 536 | + | case <-closed: | |
| 537 | + | case <-time.After(20 * testIdle): | |
| 538 | + | t.Fatalf("server connection still held: %s", what) | |
| 539 | + | } | |
| 540 | + | } | |
| 541 | + | ||
| 542 | + | // The handler returns with a small unflushed response; net/http's final flush | |
| 543 | + | // then blocks on a client that never reads (net.Pipe has no buffering). | |
| 544 | + | func TestTransferIdleTimeout_FinalFlushToNonReadingClientIsBounded(t *testing.T) { | |
| 545 | + | ln, closed := trackedPipeServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| 546 | + | _, _ = w.Write([]byte("small")) | |
| 547 | + | })) | |
| 548 | + | conn := ln.dial() | |
| 549 | + | defer conn.Close() | |
| 550 | + | _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: x\r\n\r\n")) | |
| 551 | + | require.NoError(t, err) | |
| 552 | + | ||
| 553 | + | requireClosed(t, closed, "final response flush to a non-reading client") | |
| 554 | + | } | |
| 555 | + | ||
| 556 | + | // The client declares a body it never sends and the handler answers without | |
| 557 | + | // reading it; net/http then tries to drain the unread body. | |
| 558 | + | func TestTransferIdleTimeout_UnreadDeclaredBodyDrainIsBounded(t *testing.T) { | |
| 559 | + | ln, closed := trackedPipeServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| 560 | + | http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| 561 | + | })) | |
| 562 | + | conn := ln.dial() | |
| 563 | + | defer conn.Close() | |
| 564 | + | go func() { _, _ = io.Copy(io.Discard, conn) }() // read the response, then stay silent | |
| 565 | + | _, err := conn.Write([]byte("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 100\r\n\r\n")) | |
| 566 | + | require.NoError(t, err) | |
| 567 | + | ||
| 568 | + | requireClosed(t, closed, "drain of an unread declared request body") | |
| 569 | + | } | |
| 570 | + | ||
| 571 | + | func TestTransferIdleTimeout_UnreadDeclaredBodyDrainIsBoundedOverTCP(t *testing.T) { | |
| 572 | + | closed := make(chan struct{}) | |
| 573 | + | var once sync.Once | |
| 574 | + | srv := httptest.NewUnstartedServer(TransferIdleTimeout(testIdle)(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| 575 | + | http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| 576 | + | }))) | |
| 577 | + | srv.Config.ConnState = func(_ net.Conn, state http.ConnState) { | |
| 578 | + | if state == http.StateClosed { | |
| 579 | + | once.Do(func() { close(closed) }) | |
| 580 | + | } | |
| 581 | + | } | |
| 582 | + | srv.Start() | |
| 583 | + | defer srv.Close() | |
| 584 | + | ||
| 585 | + | conn, err := net.Dial("tcp", srv.Listener.Addr().String()) | |
| 586 | + | require.NoError(t, err) | |
| 587 | + | defer conn.Close() | |
| 588 | + | _, err = conn.Write([]byte("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 100\r\n\r\n")) | |
| 589 | + | require.NoError(t, err) | |
| 590 | + | ||
| 591 | + | requireClosed(t, closed, "drain of an unread declared request body over TCP") | |
| 592 | + | } | |
| 593 | + | ||
| 594 | + | // Leftover deadlines armed at handler return must not leak into the next | |
| 595 | + | // keep-alive request on the same connection. | |
| 596 | + | func TestTransferIdleTimeout_KeepAliveNextRequestNotAffectedByLeftoverDeadlines(t *testing.T) { | |
| 597 | + | srv := httptest.NewUnstartedServer(TransferIdleTimeout(testIdle)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 598 | + | _, _ = io.Copy(io.Discard, r.Body) | |
| 599 | + | _, _ = w.Write([]byte("ok")) | |
| 600 | + | }))) | |
| 601 | + | srv.Config.IdleTimeout = 10 * time.Second | |
| 602 | + | srv.Start() | |
| 603 | + | defer srv.Close() | |
| 604 | + | ||
| 605 | + | client := &http.Client{Transport: &http.Transport{MaxConnsPerHost: 1}} | |
| 606 | + | for i := range 3 { | |
| 607 | + | resp, err := client.Post(srv.URL, "text/plain", strings.NewReader("body")) | |
| 608 | + | require.NoError(t, err, "request %d", i) | |
| 609 | + | _, err = io.ReadAll(resp.Body) | |
| 610 | + | require.NoError(t, err) | |
| 611 | + | _ = resp.Body.Close() | |
| 612 | + | time.Sleep(2 * testIdle) // outlive the leftover deadline while the connection idles | |
| 613 | + | } | |
| 614 | + | } | |
| 615 | + | ||
| 616 | + | // Hijack flushes pending headers; it must not hold the timer's mutex while it | |
| 617 | + | // blocks on a client that does not read. | |
| 618 | + | func TestTransferIdleTimeout_HijackBlockedOnNonReadingClientIsBounded(t *testing.T) { | |
| 619 | + | hijackErr := make(chan error, 1) | |
| 620 | + | ln := servePipe(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| 621 | + | w.WriteHeader(http.StatusOK) | |
| 622 | + | conn, _, err := w.(http.Hijacker).Hijack() | |
| 623 | + | if conn != nil { | |
| 624 | + | _ = conn.Close() | |
| 625 | + | } | |
| 626 | + | hijackErr <- err | |
| 627 | + | })) | |
| 628 | + | conn := ln.dial() | |
| 629 | + | defer conn.Close() | |
| 630 | + | _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: x\r\n\r\n")) | |
| 631 | + | require.NoError(t, err) | |
| 632 | + | ||
| 633 | + | select { | |
| 634 | + | case <-hijackErr: | |
| 635 | + | case <-time.After(20 * testIdle): | |
| 636 | + | t.Fatal("Hijack blocked on a non-reading client was not bounded") | |
| 637 | + | } | |
| 638 | + | } | |
| @@ -452,19 +452,23 @@ func uploadUntilEarlyResponse(t *testing.T, front *httptest.Server) { | |||||
| 452 | _ = pw.Close() | 452 | _ = pw.Close() | ||
| 453 | }() | 453 | }() | ||
| 454 | 454 | ||||
| 455 | + | defer func() { | |||
| 456 | + | _ = pr.CloseWithError(io.ErrClosedPipe) | |||
| 457 | + | <-writerDone | |||
| 458 | + | }() | |||
| 459 | + | ||||
| 455 | req, err := http.NewRequest(http.MethodPost, front.URL, pr) | 460 | req, err := http.NewRequest(http.MethodPost, front.URL, pr) | ||
| 456 | if !assert.NoError(t, err) { | 461 | if !assert.NoError(t, err) { | ||
| 457 | return | 462 | return | ||
| 458 | } | 463 | } | ||
| 459 | resp, err := front.Client().Do(req) | 464 | resp, err := front.Client().Do(req) | ||
| 460 | - | if err == nil { | 465 | + | if !assert.NoError(t, err, "the early 413 must reach the client") { |
| 461 | - | assert.Equal(t, 2, resp.ProtoMajor) | 466 | + | return |
| 462 | - | assert.Equal(t, http.StatusRequestEntityTooLarge, resp.StatusCode) | |||
| 463 | - | _, _ = io.Copy(io.Discard, resp.Body) | |||
| 464 | - | _ = resp.Body.Close() | |||
| 465 | } | 467 | } | ||
| 466 | - | _ = pr.CloseWithError(io.ErrClosedPipe) | 468 | + | defer resp.Body.Close() |
| 467 | - | <-writerDone | 469 | + | assert.Equal(t, 2, resp.ProtoMajor) |
| 470 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, resp.StatusCode) | |||
| 471 | + | _, _ = io.Copy(io.Discard, resp.Body) | |||
| 468 | } | 472 | } | ||
| 469 | 473 | ||||
| 470 | // TestTransferIdleTimeout_HTTP2BodyUsedAfterHandlerReturned uses the request | 474 | // TestTransferIdleTimeout_HTTP2BodyUsedAfterHandlerReturned uses the request | ||
| @@ -504,3 +508,131 @@ func TestTransferIdleTimeout_HTTP2BodyUsedAfterHandlerReturned(t *testing.T) { | |||||
| 504 | require.NoError(t, body.Close()) | 508 | require.NoError(t, body.Close()) | ||
| 505 | require.NoError(t, body.Close()) | 509 | require.NoError(t, body.Close()) | ||
| 506 | } | 510 | } | ||
| 511 | + | ||||
| 512 | + | // trackedPipeServer serves net.Pipe connections and reports when the server | |||
| 513 | + | // side closes one, so tests can assert a stalled connection is released. | |||
| 514 | + | func trackedPipeServer(t *testing.T, handler http.Handler) (*pipeListener, <-chan struct{}) { | |||
| 515 | + | t.Helper() | |||
| 516 | + | closed := make(chan struct{}) | |||
| 517 | + | var once sync.Once | |||
| 518 | + | ln := newPipeListener() | |||
| 519 | + | srv := &http.Server{ | |||
| 520 | + | Handler: TransferIdleTimeout(testIdle)(handler), | |||
| 521 | + | ReadHeaderTimeout: 5 * time.Second, | |||
| 522 | + | ConnState: func(_ net.Conn, state http.ConnState) { | |||
| 523 | + | if state == http.StateClosed { | |||
| 524 | + | once.Do(func() { close(closed) }) | |||
| 525 | + | } | |||
| 526 | + | }, | |||
| 527 | + | } | |||
| 528 | + | go func() { _ = srv.Serve(ln) }() | |||
| 529 | + | t.Cleanup(func() { _ = srv.Close() }) | |||
| 530 | + | return ln, closed | |||
| 531 | + | } | |||
| 532 | + | ||||
| 533 | + | func requireClosed(t *testing.T, closed <-chan struct{}, what string) { | |||
| 534 | + | t.Helper() | |||
| 535 | + | select { | |||
| 536 | + | case <-closed: | |||
| 537 | + | case <-time.After(20 * testIdle): | |||
| 538 | + | t.Fatalf("server connection still held: %s", what) | |||
| 539 | + | } | |||
| 540 | + | } | |||
| 541 | + | ||||
| 542 | + | // The handler returns with a small unflushed response; net/http's final flush | |||
| 543 | + | // then blocks on a client that never reads (net.Pipe has no buffering). | |||
| 544 | + | func TestTransferIdleTimeout_FinalFlushToNonReadingClientIsBounded(t *testing.T) { | |||
| 545 | + | ln, closed := trackedPipeServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |||
| 546 | + | _, _ = w.Write([]byte("small")) | |||
| 547 | + | })) | |||
| 548 | + | conn := ln.dial() | |||
| 549 | + | defer conn.Close() | |||
| 550 | + | _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: x\r\n\r\n")) | |||
| 551 | + | require.NoError(t, err) | |||
| 552 | + | ||||
| 553 | + | requireClosed(t, closed, "final response flush to a non-reading client") | |||
| 554 | + | } | |||
| 555 | + | ||||
| 556 | + | // The client declares a body it never sends and the handler answers without | |||
| 557 | + | // reading it; net/http then tries to drain the unread body. | |||
| 558 | + | func TestTransferIdleTimeout_UnreadDeclaredBodyDrainIsBounded(t *testing.T) { | |||
| 559 | + | ln, closed := trackedPipeServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |||
| 560 | + | http.Error(w, "unauthorized", http.StatusUnauthorized) | |||
| 561 | + | })) | |||
| 562 | + | conn := ln.dial() | |||
| 563 | + | defer conn.Close() | |||
| 564 | + | go func() { _, _ = io.Copy(io.Discard, conn) }() // read the response, then stay silent | |||
| 565 | + | _, err := conn.Write([]byte("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 100\r\n\r\n")) | |||
| 566 | + | require.NoError(t, err) | |||
| 567 | + | ||||
| 568 | + | requireClosed(t, closed, "drain of an unread declared request body") | |||
| 569 | + | } | |||
| 570 | + | ||||
| 571 | + | func TestTransferIdleTimeout_UnreadDeclaredBodyDrainIsBoundedOverTCP(t *testing.T) { | |||
| 572 | + | closed := make(chan struct{}) | |||
| 573 | + | var once sync.Once | |||
| 574 | + | srv := httptest.NewUnstartedServer(TransferIdleTimeout(testIdle)(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |||
| 575 | + | http.Error(w, "unauthorized", http.StatusUnauthorized) | |||
| 576 | + | }))) | |||
| 577 | + | srv.Config.ConnState = func(_ net.Conn, state http.ConnState) { | |||
| 578 | + | if state == http.StateClosed { | |||
| 579 | + | once.Do(func() { close(closed) }) | |||
| 580 | + | } | |||
| 581 | + | } | |||
| 582 | + | srv.Start() | |||
| 583 | + | defer srv.Close() | |||
| 584 | + | ||||
| 585 | + | conn, err := net.Dial("tcp", srv.Listener.Addr().String()) | |||
| 586 | + | require.NoError(t, err) | |||
| 587 | + | defer conn.Close() | |||
| 588 | + | _, err = conn.Write([]byte("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 100\r\n\r\n")) | |||
| 589 | + | require.NoError(t, err) | |||
| 590 | + | ||||
| 591 | + | requireClosed(t, closed, "drain of an unread declared request body over TCP") | |||
| 592 | + | } | |||
| 593 | + | ||||
| 594 | + | // Leftover deadlines armed at handler return must not leak into the next | |||
| 595 | + | // keep-alive request on the same connection. | |||
| 596 | + | func TestTransferIdleTimeout_KeepAliveNextRequestNotAffectedByLeftoverDeadlines(t *testing.T) { | |||
| 597 | + | srv := httptest.NewUnstartedServer(TransferIdleTimeout(testIdle)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 598 | + | _, _ = io.Copy(io.Discard, r.Body) | |||
| 599 | + | _, _ = w.Write([]byte("ok")) | |||
| 600 | + | }))) | |||
| 601 | + | srv.Config.IdleTimeout = 10 * time.Second | |||
| 602 | + | srv.Start() | |||
| 603 | + | defer srv.Close() | |||
| 604 | + | ||||
| 605 | + | client := &http.Client{Transport: &http.Transport{MaxConnsPerHost: 1}} | |||
| 606 | + | for i := range 3 { | |||
| 607 | + | resp, err := client.Post(srv.URL, "text/plain", strings.NewReader("body")) | |||
| 608 | + | require.NoError(t, err, "request %d", i) | |||
| 609 | + | _, err = io.ReadAll(resp.Body) | |||
| 610 | + | require.NoError(t, err) | |||
| 611 | + | _ = resp.Body.Close() | |||
| 612 | + | time.Sleep(2 * testIdle) // outlive the leftover deadline while the connection idles | |||
| 613 | + | } | |||
| 614 | + | } | |||
| 615 | + | ||||
| 616 | + | // Hijack flushes pending headers; it must not hold the timer's mutex while it | |||
| 617 | + | // blocks on a client that does not read. | |||
| 618 | + | func TestTransferIdleTimeout_HijackBlockedOnNonReadingClientIsBounded(t *testing.T) { | |||
| 619 | + | hijackErr := make(chan error, 1) | |||
| 620 | + | ln := servePipe(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |||
| 621 | + | w.WriteHeader(http.StatusOK) | |||
| 622 | + | conn, _, err := w.(http.Hijacker).Hijack() | |||
| 623 | + | if conn != nil { | |||
| 624 | + | _ = conn.Close() | |||
| 625 | + | } | |||
| 626 | + | hijackErr <- err | |||
| 627 | + | })) | |||
| 628 | + | conn := ln.dial() | |||
| 629 | + | defer conn.Close() | |||
| 630 | + | _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: x\r\n\r\n")) | |||
| 631 | + | require.NoError(t, err) | |||
| 632 | + | ||||
| 633 | + | select { | |||
| 634 | + | case <-hijackErr: | |||
| 635 | + | case <-time.After(20 * testIdle): | |||
| 636 | + | t.Fatal("Hijack blocked on a non-reading client was not bounded") | |||
| 637 | + | } | |||
| 638 | + | } | |||