feat: add Timeout middleware#45
Conversation
paskal
force-pushed
the
timeout-middleware
branch
from
9ab6040 to
7d10733
Compare
|
Addressed a review finding: the deferred 504 could become a superfluous Note: the red |
Coverage Report for CI Build 28485193325Coverage increased (+0.2%) to 96.202%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
paskal
force-pushed
the
timeout-middleware
branch
from
51eb7fd to
99a7300
Compare
|
Rebased onto master now that #44 is merged (the duplicate golangci-lint commit dropped automatically — patch already upstream), so this is now a single clean commit. Also fixed the red |
There was a problem hiding this comment.
Pull request overview
Adds a new Timeout middleware to the rest package to align with the existing middleware set (e.g., Throttle, RealIP, NoCache, etc.) by canceling the request context after a configured duration and (when possible) returning HTTP 504 if the deadline is exceeded before a response is written.
Changes:
- Introduces
Timeout(timeout time.Duration)middleware that wraps the request context withcontext.WithTimeoutand conditionally writesStatusGatewayTimeout(504). - Adds a
timeoutWriterresponse-writer wrapper and a comprehensiveTestTimeoutsuite covering core behaviors and interface forwarding. - Documents the new middleware usage and semantics in
README.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| timeout.go | Implements Timeout middleware and timeoutWriter wrapper to track whether a response has been committed. |
| timeout_test.go | Adds tests for timeout behavior, committed-response handling, and forwarding of Flusher/Hijacker. |
| README.md | Documents Timeout middleware semantics and usage example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
paskal
force-pushed
the
timeout-middleware
branch
from
99a7300 to
3c3a45f
Compare
paskal
force-pushed
the
timeout-middleware
branch
from
3c3a45f to
3727835
Compare
paskal
force-pushed
the
timeout-middleware
branch
from
3727835 to
ed95096
Compare
|
Reworked the middleware from a cooperative design (504 only after a context-respecting handler returns, chi-style) to an enforcing one modeled on This addresses the earlier review points (the buffered/enforcing model removes the superfluous-504, Flush/Hijack-commit, Unwrap and 1xx/101 concerns entirely). Trade-off, documented: buffering means Verified: 100% coverage of timeout.go, |
Enforces a maximum request duration: the next handler runs in a goroutine with a context deadline, and if it has not finished by the deadline the middleware responds with 504 Gateway Timeout at the deadline — even for a handler that ignores the context. Modeled on net/http.TimeoutHandler: the handler's output is buffered and written through on success; if the deadline fires first the buffered output is discarded, a 504 is sent, and further handler writes return http.ErrHandlerTimeout. A canceled parent context stops the handler without sending a 504. Panics propagate. Because the response is buffered, http.Flusher and http.Hijacker are not supported (documented).
paskal
force-pushed
the
timeout-middleware
branch
from
ed95096 to
00c5b21
Compare
|
Re the latest points (now outdated after the rewrite): the Flush/Hijack-commit, deferred-504-after-return, 1xx and 101 concerns are all resolved by the move to the enforcing/buffered model (there is no committed-tracking or deferred-504-after-commit anymore, and Flush/Hijack are intentionally not supported). The README now matches the behaviour. I did add the remaining valid point — a non-positive timeout now disables the middleware (calls the handler directly, no 504), like chi's |
| case <-ctx.Done(): | ||
| tw.mu.Lock() | ||
| defer tw.mu.Unlock() | ||
| tw.timedOut = true // discard the buffer and stop further handler writes | ||
| // only the deadline yields a 504; a canceled parent means the request is | ||
| // being abandoned, so there is nobody to send a status to | ||
| if ctx.Err() == context.DeadlineExceeded { | ||
| w.WriteHeader(http.StatusGatewayTimeout) | ||
| } | ||
| } |
| assert.Equal(t, http.StatusGatewayTimeout, rec.Code) | ||
| assert.Empty(t, rec.Body.String(), "the handler's buffered output must be discarded") | ||
| assert.Less(t, elapsed, 2*time.Second, "ServeHTTP must return at the deadline, not wait for the handler") | ||
|
|
Summary
go-pkgz/rest has
Throttle,RealIP,NoCache,CORS,Recovererand friends, but noTimeoutmiddleware, so consumers hand-roll one. This addsTimeout.It enforces a maximum request duration, modeled on
net/http.TimeoutHandler: the next handler runs in a goroutine with a context deadline, and if it has not finished by the deadline the middleware responds withStatusGatewayTimeout(504) at the deadline — even for a handler that ignores the context.The handler's output is buffered and written through on success; if the deadline fires first the buffered output is discarded, a 504 is sent, and further handler writes return
http.ErrHandlerTimeout. A canceled parent context stops the handler without sending a 504, and panics propagate. Because the response is buffered,http.Flusherandhttp.Hijackerare not available underTimeout(same trade-off asnet/http.TimeoutHandler).Tests
TestTimeout(subtests, millisecond deadlines,-raceclean, 100% coverage of timeout.go) covers: fast pass-through with status/headers/body; default 200; the context carrying the deadline; a context-ignoring handler timed out at the deadline (output discarded); a context-respecting handler timing out; partial output discarded on timeout; post-timeout writes returninghttp.ErrHandlerTimeout; duplicate/after-timeoutWriteHeaderno-ops; panic propagation; and canceled-parent-context stopping the handler without a 504.Found while migrating umputun/remark42 off go-chi onto go-pkgz/rest.