refactor(timeout): unify and enhance timeout middleware #3275
Conversation
ReneWerner87
commented
Jan 7, 2025
•
ReneWerner87
commented
- Enhance error handling for deadline and custom errors
- Update comments for clarity and maintainability
- Combine classic context-based timeout with a Goroutine + channel approach - Support custom error list without additional parameters - Return fiber.ErrRequestTimeout for timeouts or listed errors
ReneWerner87
requested review from
gaby,
sixcolors and
efectn
and removed request for
a team
WalkthroughThe Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- Combine classic context-based timeout with a Goroutine + channel approach - Support custom error list without additional parameters - Return fiber.ErrRequestTimeout for timeouts or listed errors
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
middleware/timeout/timeout.go (3)
11-13: Enhance documentation with examples and warnings.While the documentation clearly describes the basic functionality, consider adding:
- Example usage showing how to use custom errors
- Warning about potential memory leaks with long-running handlers that exceed the timeout
- Description of the
tErrsparameter's purpose and behavior
32-39: Clarify context cancellation handling.The comment about "other context cancellations" needs clarification. Consider handling
context.Canceleddifferently fromcontext.DeadlineExceededas they represent different scenarios.if errors.Is(timeoutContext.Err(), context.DeadlineExceeded) { return fiber.ErrRequestTimeout } -// For other context cancellations, we can still treat them the same -return fiber.ErrRequestTimeout +// Handle explicit cancellation differently +if errors.Is(timeoutContext.Err(), context.Canceled) { + return fiber.ErrRequestCanceled +} +return fiber.ErrRequestTimeout
47-52: Optimize custom error checking.For better performance with a large number of custom errors, consider using a map for O(1) lookup instead of iterating through the slice.
+// Convert error slice to map at middleware creation +errorMap := make(map[error]struct{}, len(tErrs)) +for _, err := range tErrs { + errorMap[err] = struct{}{} +} -// Check against any custom errors in the list -for _, timeoutErr := range tErrs { - if errors.Is(err, timeoutErr) { - return fiber.ErrRequestTimeout - } -} +// O(1) lookup +if _, exists := errorMap[err]; exists { + return fiber.ErrRequestTimeout +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/timeout/timeout.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
middleware/timeout/timeout.go (1)
15-21: LGTM! Clean context setup with proper cleanup.The implementation follows Go best practices for context handling with proper cleanup via deferred cancellation.
…tion - Switch to a synchronous approach to avoid data races with fasthttp context - Enhance error handling for deadline and custom errors - Update comments for clarity and maintainability
ReneWerner87
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
23-39:⚠️ Potential issueAdd panic recovery and consider asynchronous execution.
The current implementation lacks panic recovery and might not handle high concurrency efficiently.
Consider this enhanced implementation:
-// Execute the wrapped handler synchronously. -err := h(ctx) +// Execute handler with panic recovery +done := make(chan error, 1) +go func() { + defer func() { + if r := recover(); r != nil { + done <- fiber.NewError(fiber.StatusInternalServerError, "panic recovered") + } + }() + done <- h(ctx) +}() + +// Wait for either timeout or handler completion +select { +case err := <-done: + if err != nil { + if errors.Is(err, context.DeadlineExceeded) || isCustomError(err, tErrs) { + return fiber.ErrRequestTimeout + } + return err + } +case <-timeoutContext.Done(): + return fiber.ErrRequestTimeout +}
🧹 Nitpick comments (2)
middleware/timeout/timeout.go (2)
11-13: Enhance documentation with concurrency safety notes.While the documentation clearly describes the timeout behavior, it should also document whether the
fiber.Ctxis safe for concurrent access, as this is critical for middleware implementers.Add the following to the documentation:
// New enforces a timeout for each incoming request. If the timeout expires or // any of the specified errors occur, fiber.ErrRequestTimeout is returned. +// +// Note: This middleware assumes that fiber.Ctx is safe for concurrent access +// between the main goroutine and timeout goroutine. Exercise caution when +// modifying the context from multiple goroutines.
42-49: Document and optimize error checking.The helper function works correctly but could benefit from optimization for large error lists and better documentation.
Consider these improvements:
-// isCustomError checks whether err matches any error in errList using errors.Is. +// isCustomError checks whether err matches any error in errList using errors.Is. +// Time complexity is O(n) where n is the length of errList. For large error lists, +// consider using a more efficient data structure if performance is critical. func isCustomError(err error, errList []error) bool { + if len(errList) == 0 { + return false + } for _, e := range errList { if errors.Is(err, e) { return true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/timeout/timeout.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/timeout/timeout.go (2)
11-49: Add comprehensive test coverage.The timeout middleware requires thorough testing to ensure reliability across various scenarios.
Please add tests in
middleware/timeout/timeout_test.gofor:
- Successful completion within timeout
- Timeout scenarios
- Custom error handling
- Context cancellation
- Panic recovery (once implemented)
Run this script to verify test coverage:
#!/bin/bash # Check test coverage for the timeout middleware # Look for timeout-related test scenarios rg "context\.WithTimeout|fiber\.ErrRequestTimeout" --type go test -A 5 # Check if panic recovery is tested rg -i "TestTimeout.*Panic" --type go test
15-22: Consider race conditions in context management.The current implementation might have a race condition if the timeout occurs between creating the context and attaching it to the Fiber context. Also, ensure proper cleanup of the Fiber context in error scenarios.
Consider this approach:
timeoutContext, cancel := context.WithTimeout(ctx.Context(), timeout) defer cancel() + +// Store the original context to restore it in case of errors +originalContext := ctx.Context() +defer func() { + // Restore the original context to prevent leaks + ctx.SetContext(originalContext) +}() // Attach the timeout-bound context to the current Fiber context. ctx.SetContext(timeoutContext)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
28-29:⚠️ Potential issueAdd panic recovery for handler execution.
The handler execution should be wrapped in a panic recovery to prevent crashes.
// Execute the wrapped handler synchronously. +err := func() (err error) { + defer func() { + if r := recover(); r != nil { + err = fiber.NewError(fiber.StatusInternalServerError, "panic recovered") + } + }() + return h(ctx) +}() -err := h(ctx)
🧹 Nitpick comments (3)
middleware/timeout/timeout.go (1)
11-13: Enhance documentation for custom error handling.While the documentation is clear about the basic timeout functionality, it could be more descriptive about the
tErrsparameter and how custom errors are handled.// New enforces a timeout for each incoming request. If the timeout expires or -// any of the specified errors occur, fiber.ErrRequestTimeout is returned. +// any of the specified errors occur, fiber.ErrRequestTimeout is returned. +// Custom errors (tErrs) allow treating specific errors as timeout conditions, +// useful for scenarios where operations may fail with domain-specific timeout errors. func New(h fiber.Handler, timeout time.Duration, tErrs ...error) fiber.Handler {middleware/timeout/timeout_test.go (2)
36-70: Enhance test coverage with additional assertions.While the basic scenarios are covered, consider adding:
- Response body assertions
- Edge cases (e.g., timeout-1ms)
- Headers verification
110-124: Add test for negative timeout.Consider adding a test case for negative timeout duration to ensure complete coverage of edge cases.
func TestTimeout_NegativeDuration(t *testing.T) { app := fiber.New() app.Get("/negative", New(func(c fiber.Ctx) error { time.Sleep(50 * time.Millisecond) return c.SendString("No timeout used") }, -1*time.Millisecond)) req := httptest.NewRequest(fiber.MethodGet, "/negative", nil) resp, err := app.Test(req) require.NoError(t, err) require.Equal(t, fiber.StatusOK, resp.StatusCode) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (5)
middleware/timeout/timeout.go (2)
15-18: LGTM! Efficient handling of zero timeout case.Good optimization to skip context creation when timeout is zero or negative.
47-64: Add unit tests for helper functions.While the helper functions are well-implemented, they should have dedicated unit tests to verify their behavior in isolation.
Run this script to check current test coverage:
middleware/timeout/timeout_test.go (3)
15-21: LGTM! Well-structured test error declarations.Clear separation and documentation of test error cases.
23-34: LGTM! Well-implemented test helper.The
sleepWithContextfunction is well-designed with proper timer cleanup and clear error handling.
73-108: LGTM! Comprehensive error handling tests.Excellent coverage of both custom timeout errors and unmatched errors, including wrapped errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/timeout/timeout_test.go (2)
75-94: Consider adding error message verification.While the test effectively verifies custom error handling, it could be enhanced by checking the error message in the response body to ensure proper error wrapping.
resp, err := app.Test(req) require.NoError(t, err, "app.Test(req) should not fail") require.Equal(t, fiber.StatusRequestTimeout, resp.StatusCode, "Expected 408 for custom timeout error") + body, _ := io.ReadAll(resp.Body) + require.Contains(t, string(body), "wrapped: custom timeout error", "Expected wrapped error message in response")
114-129: Consider adding duration verification.While the test verifies the zero timeout case, it could be enhanced by ensuring the full duration was actually consumed.
app.Get("/zero", New(func(c fiber.Ctx) error { + start := time.Now() // Sleep 50ms, but there's no real 'deadline' since zero-timeout. time.Sleep(50 * time.Millisecond) + duration := time.Since(start) + require.GreaterOrEqual(t, duration, 50*time.Millisecond, "Expected full sleep duration") return c.SendString("No timeout used") }, 0))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/timeout/timeout_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (6)
middleware/timeout/timeout_test.go (6)
15-21: LGTM! Well-structured error definitions.The error variables are well-documented and clearly separate timeout-related errors from unrelated ones, which is crucial for testing different error scenarios.
23-34: LGTM! Clean implementation of the helper function.The
sleepWithContextfunction is well-implemented with:
- Proper timer cleanup using defer
- Clear error propagation
- Effective use of select for context cancellation
36-54: LGTM! Well-structured success test case.The test effectively verifies the happy path with:
- Clear timing constraints (10ms operation vs 50ms timeout)
- Proper error handling
- Explicit status code verification
56-73: LGTM! Comprehensive timeout test.The test properly verifies timeout behavior with:
- Clear timing setup (200ms operation vs 100ms timeout)
- Appropriate status code verification (408)
96-112: LGTM! Good error differentiation test.The test effectively verifies that unmatched errors result in 500 status, maintaining proper error handling hierarchy.
36-129: LGTM! Comprehensive test coverage.The test suite effectively covers all critical scenarios:
- Success cases
- Timeout handling
- Custom error handling
- Error differentiation
- Edge cases (zero duration)
The consistent use of
t.Parallel()and clear documentation makes the tests both efficient and maintainable.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3275 +/- ##
==========================================
+ Coverage 84.04% 84.15% +0.10%
==========================================
Files 116 116
Lines 11522 11541 +19
==========================================
+ Hits 9684 9712 +28
+ Misses 1407 1400 -7
+ Partials 431 429 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
28-35:⚠️ Potential issueAdd panic recovery in the handler execution.
The handler execution should include panic recovery to prevent crashes.
This is a duplicate of a previous review comment. The concern about panic recovery is still valid and should be addressed.
🧹 Nitpick comments (2)
middleware/timeout/timeout.go (2)
11-13: Enhance function documentation with examples and parameter details.While the documentation is clear, it would be more helpful with:
- Parameter descriptions (timeout duration, custom errors)
- Usage examples
- Return value documentation
Add the following to the documentation:
// New enforces a timeout for each incoming request. If the timeout expires or // any of the specified errors occur, fiber.ErrRequestTimeout is returned. +// +// Parameters: +// - h: The handler to be executed with timeout enforcement +// - timeout: Maximum duration to wait for handler completion. Use <= 0 to disable timeout +// - tErrs: Optional list of custom errors that should be treated as timeout errors +// +// Returns: +// fiber.Handler that enforces the specified timeout +// +// Example: +// +// app.Get("/", timeout.New(handler, 5*time.Second)) +// +// // With custom timeout errors +// app.Get("/", timeout.New(handler, 5*time.Second, ErrDatabaseTimeout, ErrRedisTimeout))
51-58: Consider optimizing error list lookup for large error sets.While the current implementation is clean and correct, for cases with many custom errors, consider using a more efficient lookup method.
For large error lists, you could use a map-based approach:
+// isCustomError checks whether err matches any error in errList using errors.Is. +// For large error lists, it uses a map for efficient lookup. func isCustomError(err error, errList []error) bool { + if len(errList) > 10 { // threshold for switching to map-based lookup + errorMap := make(map[error]struct{}, len(errList)) + for _, e := range errList { + errorMap[e] = struct{}{} + } + for e := range errorMap { + if errors.Is(err, e) { + return true + } + } + return false + } for _, e := range errList { if errors.Is(err, e) { return true } } return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/timeout/timeout.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (2)
middleware/timeout/timeout.go (2)
39-49: Add test coverage for helper functions.The new helper functions need comprehensive test coverage to verify behavior with:
- Various error scenarios
- Different combinations of custom errors
- Edge cases
This is a duplicate of a previous review comment about test coverage. The need for tests is even more important now with the addition of these helper functions.
25-26: Review concurrent context modification safety.The
SetContextcall modifies the Fiber context, which might not be safe for concurrent access. Consider documenting thread-safety guarantees or potential limitations.Let's check the Fiber documentation and codebase for context safety:
