-
Notifications
You must be signed in to change notification settings - Fork 129
feat(gateway): add configurable response write timeout #812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ | |||||||||||||||||||||||||||||
"regexp" | ||||||||||||||||||||||||||||||
"runtime/debug" | ||||||||||||||||||||||||||||||
"strings" | ||||||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
"github.com/ipfs/boxo/gateway/assets" | ||||||||||||||||||||||||||||||
|
@@ -68,7 +69,83 @@ | |||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||
// [IPFS HTTP Gateway]: https://specs.ipfs.tech/http-gateways/ | ||||||||||||||||||||||||||||||
func NewHandler(c Config, backend IPFSBackend) http.Handler { | ||||||||||||||||||||||||||||||
return newHandlerWithMetrics(&c, backend) | ||||||||||||||||||||||||||||||
handler := newHandlerWithMetrics(&c, backend) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// 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 | ||||||||||||||||||||||||||||||
gitsrc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Apply the timeout middleware | ||||||||||||||||||||||||||||||
return WithResponseWriteTimeout(handler, timeout) | ||||||||||||||||||||||||||||||
gitsrc marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+74
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// timeoutResponseWriter implements http.ResponseWriter with timeout control | ||||||||||||||||||||||||||||||
type timeoutResponseWriter struct { | ||||||||||||||||||||||||||||||
http.ResponseWriter // Embedded standard response writer | ||||||||||||||||||||||||||||||
timer *time.Timer // Timeout tracking mechanism | ||||||||||||||||||||||||||||||
timeout time.Duration // Configured timeout duration | ||||||||||||||||||||||||||||||
mu sync.Mutex // Mutex for concurrent access protection | ||||||||||||||||||||||||||||||
done bool // Completion state flag | ||||||||||||||||||||||||||||||
requestCtx context.Context // Original request context | ||||||||||||||||||||||||||||||
gitsrc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
cancel context.CancelFunc // Context cancellation handler | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Write implements io.Writer interface with timeout reset functionality | ||||||||||||||||||||||||||||||
func (w *timeoutResponseWriter) Write(data []byte) (int, error) { | ||||||||||||||||||||||||||||||
w.mu.Lock() | ||||||||||||||||||||||||||||||
defer w.mu.Unlock() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if w.done { | ||||||||||||||||||||||||||||||
return 0, http.ErrHandlerTimeout | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Reset timer on successful write attempt | ||||||||||||||||||||||||||||||
w.timer.Reset(w.timeout) | ||||||||||||||||||||||||||||||
return w.ResponseWriter.Write(data) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// cancelRequest handles timeout termination sequence | ||||||||||||||||||||||||||||||
func (w *timeoutResponseWriter) cancelRequest() { | ||||||||||||||||||||||||||||||
w.mu.Lock() | ||||||||||||||||||||||||||||||
defer w.mu.Unlock() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if !w.done { | ||||||||||||||||||||||||||||||
w.done = true | ||||||||||||||||||||||||||||||
w.cancel() // Propagate context cancellation | ||||||||||||||||||||||||||||||
w.ResponseWriter.WriteHeader(http.StatusGatewayTimeout) | ||||||||||||||||||||||||||||||
gitsrc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// WithResponseWriteTimeout creates middleware for response write timeout handling | ||||||||||||||||||||||||||||||
func WithResponseWriteTimeout(next http.Handler, timeout time.Duration) http.Handler { | ||||||||||||||||||||||||||||||
Comment on lines
+121
to
+122
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not appear to need to be exported.
Suggested change
|
||||||||||||||||||||||||||||||
return http.HandlerFunc(func(origWriter http.ResponseWriter, r *http.Request) { | ||||||||||||||||||||||||||||||
// Create derived context with cancellation capability | ||||||||||||||||||||||||||||||
ctx, cancel := context.WithCancel(r.Context()) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
gitsrc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
// Initialize enhanced response writer | ||||||||||||||||||||||||||||||
tw := &timeoutResponseWriter{ | ||||||||||||||||||||||||||||||
ResponseWriter: origWriter, | ||||||||||||||||||||||||||||||
timeout: timeout, | ||||||||||||||||||||||||||||||
timer: time.NewTimer(timeout), | ||||||||||||||||||||||||||||||
requestCtx: ctx, | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not used here
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, why we need requestCtx @gitsrc, does not seem to be used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, this variable is not used and can be deleted. |
||||||||||||||||||||||||||||||
cancel: cancel, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
defer tw.timer.Stop() // Ensure timer resource cleanup | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Concurrent timeout monitor | ||||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||||
select { | ||||||||||||||||||||||||||||||
case <-tw.timer.C: // Timeout expiration | ||||||||||||||||||||||||||||||
tw.cancelRequest() | ||||||||||||||||||||||||||||||
case <-ctx.Done(): // Normal completion | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Execute handler chain with wrapped context | ||||||||||||||||||||||||||||||
next.ServeHTTP(tw, r.WithContext(ctx)) | ||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// serveContent replies to the request using the content in the provided Reader | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: run gofmt on this file |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
package gateway | ||
|
||
import ( | ||
"net/http" | ||
"net/http/httptest" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
@@ -28,3 +32,90 @@ | |
assert.Equalf(t, test.expected, result, "etagMatch(%q, %q, %q)", test.header, test.cidEtag, test.dirEtag) | ||
} | ||
} | ||
|
||
|
||
|
||
func TestWithResponseWriteTimeout(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
handler http.HandlerFunc // Test scenario handler implementation | ||
timeout time.Duration // Timeout configuration for middleware | ||
expectStatus int // Anticipated HTTP response code | ||
expectedChunks int // Expected number of completed writes | ||
}{ | ||
{ | ||
name: "Normal completion - write within timeout", | ||
handler: func(w http.ResponseWriter, r *http.Request) { | ||
select { | ||
case <-time.After(100 * time.Millisecond): | ||
w.Write([]byte("chunk\n")) | ||
case <-r.Context().Done(): | ||
} | ||
}, | ||
timeout: 300 * time.Millisecond, | ||
expectStatus: http.StatusOK, | ||
expectedChunks: 1, | ||
}, | ||
{ | ||
name: "Timeout triggered - write after deadline", | ||
handler: func(w http.ResponseWriter, r *http.Request) { | ||
select { | ||
case <-time.After(400 * time.Millisecond): // Exceeds 300ms timeout | ||
w.Write([]byte("chunk\n")) // Should be post-timeout | ||
case <-r.Context().Done(): | ||
} | ||
}, | ||
timeout: 300 * time.Millisecond, | ||
expectStatus: http.StatusGatewayTimeout, | ||
expectedChunks: 0, | ||
}, | ||
{ | ||
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, | ||
}, | ||
Comment on lines
+72
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Initialize test components | ||
req := httptest.NewRequest("GET", "http://example.com", nil) | ||
rec := httptest.NewRecorder() | ||
|
||
// Configure middleware chain | ||
wrappedHandler := WithResponseWriteTimeout(tt.handler, tt.timeout) | ||
wrappedHandler.ServeHTTP(rec, req) | ||
|
||
// Validate HTTP status code | ||
if rec.Code != tt.expectStatus { | ||
t.Errorf("Status code validation failed: expected %d, received %d", | ||
tt.expectStatus, rec.Code) | ||
} | ||
|
||
// Verify response body integrity | ||
actualChunks := strings.Count(rec.Body.String(), "chunk") | ||
if actualChunks != tt.expectedChunks { | ||
t.Errorf("Data chunk mismatch: anticipated %d, obtained %d", | ||
tt.expectedChunks, actualChunks) | ||
} | ||
|
||
// Ensure timeout responses don't set headers | ||
if tt.expectStatus == http.StatusGatewayTimeout { | ||
if contentType := rec.Header().Get("Content-Type"); contentType != "" { | ||
t.Errorf("Timeout response contains unexpected Content-Type: %s", contentType) | ||
} | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.