feat(gateway): add configurable response write timeout #812
Conversation
Signed-off-by: gitsrc <[email protected]>
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
| // WithResponseWriteTimeout creates middleware for response write timeout handling | ||
| func WithResponseWriteTimeout(next http.Handler, timeout time.Duration) http.Handler { |
There was a problem hiding this comment.
This does not appear to need to be exported.
| // WithResponseWriteTimeout creates middleware for response write timeout handling | |
| func WithResponseWriteTimeout(next http.Handler, timeout time.Duration) http.Handler { | |
| // withResponseWriteTimeout creates middleware for response write timeout handling | |
| func withResponseWriteTimeout(next http.Handler, timeout time.Duration) http.Handler { |
| ResponseWriter: origWriter, | ||
| timeout: timeout, | ||
| timer: time.NewTimer(timeout), | ||
| requestCtx: ctx, |
There was a problem hiding this comment.
not used here
| requestCtx: ctx, |
There was a problem hiding this comment.
+1, why we need requestCtx @gitsrc, does not seem to be used?
There was a problem hiding this comment.
You are right, this variable is not used and can be deleted.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #812 +/- ##
==========================================
- Coverage 60.48% 60.44% -0.04%
==========================================
Files 244 243 -1
Lines 31121 31147 +26
==========================================
+ Hits 18822 18827 +5
- Misses 10623 10639 +16
- Partials 1676 1681 +5
|
|
Triage note:
|
| // ResponseWriteTimeout is the maximum duration the gateway will wait for a response | ||
| // to be written before timing out and returning a 504 Gateway Timeout error. | ||
| // If not set, a default timeout will be used. |
There was a problem hiding this comment.
| // ResponseWriteTimeout is the maximum duration the gateway will wait for a response | |
| // to be written before timing out and returning a 504 Gateway Timeout error. | |
| // If not set, a default timeout will be used. | |
| // ResponseWriteTimeout is the maximum duration the gateway will wait for | |
| // new bytes to be retrieved from [gateway.IPFSBackend]. This timeout is | |
| // reset on every [http.ResponseWriter] Write, which is protecting both | |
| // client and server from wasting resources when parts of requested file or | |
| // DAG have no providers and would hang forever. | |
| // Setting to 0 disables this timeout. |
| { | ||
| name: "Timer reset with staggered writes", | ||
| handler: func(w http.ResponseWriter, r *http.Request) { | ||
| for i := 0; i < 3; i++ { | ||
| select { | ||
| case <-time.After(200 * time.Millisecond): // Each write within timeout window | ||
| w.Write([]byte("chunk\n")) // Resets timer on each write | ||
| case <-r.Context().Done(): | ||
| return | ||
| } | ||
| } | ||
| }, | ||
| timeout: 300 * time.Millisecond, | ||
| expectStatus: http.StatusOK, | ||
| expectedChunks: 3, | ||
| }, |
There was a problem hiding this comment.
Missing test like this, but that writes two chunks and timeouts on third.
We need to see what happens when Headers were already sent with status 200, but payload truncated due to timeout.
Perhaps it would be useful to w.Write([]byte("\n[TRUNCATED DUE TO gateway.IPFSBackend TIMEOUT]")) to make it easier for clients to understand what happened if they debug things?
| // Use the configured timeout or fall back to a default value | ||
| timeout := c.ResponseWriteTimeout | ||
| if timeout == 0 { | ||
| timeout = 30 * time.Second // Default timeout of 30 seconds | ||
| } | ||
|
|
||
| // Apply the timeout middleware | ||
| return WithResponseWriteTimeout(handler, timeout) |
There was a problem hiding this comment.
Hm.. this makes disabling timeout impossible.
Let's change this to no timeout by default (Kubo and Rainbow will set the default themselves), and skip entire things if no timeout.
| // Use the configured timeout or fall back to a default value | |
| timeout := c.ResponseWriteTimeout | |
| if timeout == 0 { | |
| timeout = 30 * time.Second // Default timeout of 30 seconds | |
| } | |
| // Apply the timeout middleware | |
| return WithResponseWriteTimeout(handler, timeout) | |
| if c.ResponseWriteTimeout != 0 { | |
| handler = WithResponseWriteTimeout(handler, timeout) | |
| } | |
| // Apply the timeout middleware | |
| return handler |
| ResponseWriter: origWriter, | ||
| timeout: timeout, | ||
| timer: time.NewTimer(timeout), | ||
| requestCtx: ctx, |
There was a problem hiding this comment.
+1, why we need requestCtx @gitsrc, does not seem to be used?
This commit introduces a configurable response write timeout for the IPFS gateway.
The timeout can be set via the
ResponseWriteTimeoutfield in the gateway configuration.If not set, a default timeout of 30 seconds is applied.
The implementation includes:
timeoutResponseWriterstruct that wraps the standardhttp.ResponseWriterand enforces the timeout.
WithResponseWriteTimeoutthat applies the timeout logicto the HTTP handler chain.
The timeout ensures that slow or unresponsive clients do not indefinitely hold
server resources, improving the overall reliability and stability of the gateway.
This change also attempts to address the issue described in #679
by providing a mechanism to handle slow or stuck HTTP responses more gracefully.