fix(timeout): Return immediately on timeout and propagate context correctly#4009
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change reworks the timeout middleware to run handlers in a separate goroutine and introduce an "Abandon" lifecycle for contexts: on timeout the middleware returns immediately (using OnTimeout or 408), marks the context abandoned, lets the handler finish asynchronously, and provides cleanup/force-release APIs ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as Timeout Middleware
participant Handler as Handler Goroutine
participant Cleanup as Cleanup Goroutine
participant Pool as Context Pool
Client->>Middleware: HTTP Request
Middleware->>Middleware: start timeout timer
Middleware->>Handler: start handler in goroutine (ctx with timeout)
alt Handler completes before timeout
Handler->>Middleware: handler result (or panic)
Middleware->>Client: return handler response (or 500 on panic)
Middleware->>Pool: release context (abandoned == false)
else Timeout expires first
Middleware->>Middleware: call Abandon() on ctx, mark timed out
Middleware->>Client: return OnTimeout result or 408 immediately
Middleware->>Cleanup: spawn cleanup goroutine to wait for handler
Handler->>Handler: continues running (can observe c.Context().Done())
Handler->>Cleanup: handler finishes
Cleanup->>Cleanup: clear abandoned flag / call ForceRelease()
Cleanup->>Pool: return context to pool
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of fixing the timeout middleware's behavior to return immediately and propagate context correctly. The changes are well-documented and thoroughly tested. My review includes a suggestion to refactor the main select block in the middleware. This refactoring aims to reduce code duplication, ensure the OnTimeout handler receives the correct context, and make the logic for handling timeouts more consistent and maintainable.
There was a problem hiding this comment.
Pull request overview
This PR fixes timeout middleware behavior to return immediately when a timeout expires (previously waited for handler completion) and ensures proper context propagation so handlers can detect cancellation. It also adds panic recovery that converts handler panics to 500 errors.
Changes:
- Handler execution now runs in a goroutine with select-based timeout handling
- Panic recovery added with conversion to
fiber.ErrInternalServerError - Test coverage enhanced with new tests for immediate return, context propagation, and panic handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| middleware/timeout/timeout.go | Refactored to run handler in goroutine with select statement for immediate timeout response; added panic recovery; merged helper functions into isTimeoutError |
| middleware/timeout/timeout_test.go | Added comprehensive tests for immediate return, context propagation, panic handling, and converted shared state variables to atomic types for thread safety |
| docs/middleware/timeout.md | Added documentation about immediate return behavior, context cancellation detection, and panic handling |
| docs/whats_new.md | Added behavioral change notes in two sections documenting immediate response, context propagation, and panic handling |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@middleware/timeout/timeout_test.go`:
- Around line 290-313: Add t.Parallel() as the first statement in the
TestIsTimeoutError_WithOnTimeout test to run it concurrently; edit the
TestIsTimeoutError_WithOnTimeout function and insert t.Parallel() immediately
after the function signature so the test follows the repository's parallel test
guideline.
🧹 Nitpick comments (1)
middleware/timeout/timeout.go (1)
64-69: Consider logging the panic value for debugging.The panic is correctly caught and converted to a 500 error, but the panic value itself is discarded. For debugging purposes, consider logging the panic value before returning.
🔧 Optional: Log panic for debugging
- case <-panicChan: + case p := <-panicChan: // Handler panicked - treat as internal server error - // We don't re-panic because we're in a different goroutine context + // Log the panic for debugging but don't re-panic + // Note: Consider using fiber's logger here if available + _ = p // panic value available for logging if needed cancel() ctx.SetContext(parent) return fiber.ErrInternalServerError
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/timeout.mddocs/whats_new.mdmiddleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
🧰 Additional context used
📓 Path-based instructions (4)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.mddocs/middleware/timeout.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/whats_new.mddocs/middleware/timeout.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/timeout/timeout_test.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
middleware/timeout/timeout_test.go
🧬 Code graph analysis (2)
middleware/timeout/timeout.go (1)
constants.go (2)
ErrRequestTimeout(126-126)ErrInternalServerError(148-148)
middleware/timeout/timeout_test.go (2)
middleware/timeout/config.go (1)
Config(10-26)constants.go (2)
StatusRequestTimeout(81-81)StatusInternalServerError(103-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cleanup artifacts
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (12)
docs/middleware/timeout.md (2)
11-15: LGTM! Documentation accurately describes the new behavior.The added documentation clearly explains:
- Immediate return on timeout expiration
- Context propagation via
c.Context().Done()- Panic handling returning 500 Internal Server Error
This aligns well with the implementation changes in
timeout.go.
17-21: Clear and important usage guidance.The caution block appropriately warns users about the middleware's constraints—specifically that it wraps the final handler and that calling
c.Next()will panic.docs/whats_new.md (2)
1577-1582: LGTM! Behavioral changes are well documented.The documentation clearly communicates the three key behavioral changes:
- Immediate response on timeout
- Context propagation via
c.Context().Done()- Panic handling returning 500 Internal Server Error
This provides users with clear expectations when upgrading.
2850-2855: Consistent with the earlier section.The migration guide appropriately repeats the behavioral changes to ensure users migrating from v2 are aware of these important differences. The content is consistent with the "What's New" section.
middleware/timeout/timeout.go (4)
33-35: Correct use of buffered channels.The buffered channels with size 1 ensure the handler goroutine can complete its send without blocking, even if the middleware has already returned due to timeout. This prevents goroutine leaks.
38-45: LGTM! Proper goroutine-based execution with panic recovery.The pattern correctly:
- Runs the handler in a separate goroutine for non-blocking timeout enforcement
- Uses
deferwithrecover()to catch panics- Sends either the panic value or handler result to the appropriate channel
This allows the middleware to return immediately on timeout regardless of handler behavior.
85-99: LGTM! Clean helper function.The
isTimeoutErrorfunction correctly:
- Uses
errors.Isto handle wrapped errors- Checks
context.DeadlineExceededfirst (most common case)- Iterates custom errors only when the slice is non-empty
71-80: This is the intended design—no changes needed.The middleware correctly handles concurrent context access. When timeout occurs, the handler goroutine may still be running, but this is expected and documented behavior. Tests (
TestTimeout_ImmediateReturn,TestTimeout_PanicAfterTimeout) confirm that:
- The response returns immediately on timeout, even if the handler continues running in the background.
- Handlers can detect the timeout by checking
c.Context().Done().- Panics or continued execution after timeout don't affect the response that was already sent.
The thread-safety is guaranteed because
fasthttp.UserValueoperations (used bySetContextandContext) are thread-safe, and the response is sent before the handler continues.middleware/timeout/timeout_test.go (4)
83-107: Excellent test for Issue#3394.This test validates the key behavioral change—that the middleware returns immediately on timeout even when the handler doesn't check the context. Good use of
atomic.Boolfor thread-safe assertion.
111-137: Good test for Issue#3671.This test validates context propagation—that handlers can detect timeout via
c.Context().Done(). Thetime.Sleep(20 * time.Millisecond)is acceptable here to allow the handler goroutine to process the cancellation before assertion.
239-269: Comprehensive panic handling tests.Both tests properly validate the panic recovery behavior:
TestTimeout_PanicInHandler: Verifies immediate panics return 500TestTimeout_PanicAfterTimeout: Verifies late panics (after timeout response) don't affect the response
316-334: LGTM! Good test for Next() skip functionality.This test verifies that when
Nextreturnstrue, the timeout middleware is bypassed and the handler runs without timeout constraints.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
=======================================
Coverage 91.17% 91.18%
=======================================
Files 119 119
Lines 10946 10987 +41
=======================================
+ Hits 9980 10018 +38
- Misses 609 612 +3
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@middleware/timeout/timeout.go`:
- Around line 47-70: Fix the misspelled words in the comments around the timeout
handling block: update comment text near the select/done/panicChan logic and the
line "// Check if timeout occurred BEFORE cancelling (cancel() would set Err())"
to use correct spelling and style (e.g., "occurred" and the preferred
"canceling" or your repo's convention), keeping the surrounding logic intact
(tCtx, done, panicChan, cancel(), ctx.SetContext(parent), panicked). Ensure
comments are concise, correctly spelled, and do not change runtime behavior.
♻️ Duplicate comments (1)
middleware/timeout/timeout_test.go (1)
272-296: Missingt.Parallel()call.Per coding guidelines, all tests should invoke
t.Parallel()at the start to maximize concurrency.🔧 Proposed fix
// TestIsTimeoutError_WithOnTimeout verifies that custom OnTimeout is called for custom errors. func TestIsTimeoutError_WithOnTimeout(t *testing.T) { + t.Parallel() app := fiber.New()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/timeout.mddocs/whats_new.mdmiddleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/timeout/timeout_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/timeout/timeout_test.gomiddleware/timeout/timeout.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/timeout.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/middleware/timeout.md
🧠 Learnings (11)
📓 Common learnings
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/timeout/timeout_test.gomiddleware/timeout/timeout.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/timeout/timeout_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
middleware/timeout/timeout_test.gomiddleware/timeout/timeout.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/timeout/timeout.go
🧬 Code graph analysis (2)
middleware/timeout/timeout_test.go (2)
middleware/timeout/timeout.go (1)
New(15-85)middleware/timeout/config.go (1)
Config(10-26)
middleware/timeout/timeout.go (1)
constants.go (2)
ErrInternalServerError(148-148)ErrRequestTimeout(126-126)
🪛 GitHub Actions: golangci-lint
middleware/timeout/timeout.go
[error] 60-60: golangci-lint: misspelling 'cancelling' should be 'canceling' (misspell).
🪛 GitHub Check: lint
middleware/timeout/timeout.go
[failure] 72-72:
cancelled is a misspelling of canceled (misspell)
[failure] 60-60:
cancelling is a misspelling of canceling (misspell)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, ubuntu-latest)
- GitHub Check: lint
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (15)
middleware/timeout/timeout_test.go (8)
40-57: LGTM!Test properly validates that handlers completing within the timeout window return successfully. Good use of
sleepWithContexthelper and propert.Parallel()invocation.
59-80: LGTM!This test correctly validates the core timeout behavior. The elapsed time check (line 79) ensures the handler returns early on context cancellation rather than waiting the full 200ms.
82-107: LGTM!Excellent test for Issue
#3671- properly validates that the timeout context is propagated to handlers so they can detect cancellation viac.Context().Done(). Good use ofatomic.Boolfor thread-safe state tracking.
109-137: LGTM!Well-designed test that demonstrates handlers can return early by checking context, making the overall request faster than the handler's potential work time.
212-236: LGTM!Good use of
atomic.Int32to safely track that the custom handler was called exactly once across goroutines.
238-252: LGTM!This test validates the new panic recovery behavior - panics in handlers are caught and translated to 500 Internal Server Error instead of crashing the process.
298-317: LGTM!Good test coverage for the
Nextfunction skip behavior, validating that whenNextreturnstrue, the middleware is bypassed entirely.
319-336: LGTM!Important test that validates timeout detection even when the handler returns
nilinstead of an error - the middleware correctly checks the context state independently.middleware/timeout/timeout.go (5)
10-14: LGTM!Clear and accurate documentation of the middleware's behavior - timeout context propagation, detection via
c.Context().Done(), and return offiber.ErrRequestTimeouton deadline exceeded.
18-26: LGTM!Good handling of the skip path (
cfg.Next) and non-positive timeout path - both execute the handler directly without the timeout orchestration overhead.
28-45: LGTM!Well-structured concurrent execution model:
- Timeout context properly created and exposed via
ctx.SetContext(tCtx)- Buffered channels (size 1) prevent goroutine leaks when the receiver has already returned
- Panic recovery in the deferred function catches panics and sends to
panicChan
72-84: LGTM!Timeout handling logic is correct:
- Checks both
contextTimedOutflag andisTimeoutErrorfor comprehensive detection- Properly invokes
OnTimeoutif configured and propagates its error- Falls back to
fiber.ErrRequestTimeoutas the default timeout response
87-101: LGTM!Clean helper function that correctly identifies timeout-like errors using
errors.Isfor proper error chain unwrapping. Handles bothcontext.DeadlineExceededand custom configured errors.docs/middleware/timeout.md (2)
11-15: LGTM!Documentation accurately reflects the new middleware behavior:
- Context cancellation detection via
c.Context().Done()- 408 response on timeout
- Panic handling returning 500 Internal Server Error
This aligns well with the implementation in
timeout.go.
50-76: LGTM!Excellent example demonstrating the recommended pattern for timeout-aware handlers using
sleepWithContext. This clearly shows how handlers should checkctx.Done()to cooperate with the timeout middleware.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the timeout middleware to correctly propagate the timeout context and to handle panics within handlers by returning a 500 error. The implementation now runs the handler in a separate goroutine to facilitate panic recovery. While this is a good improvement for robustness, I've identified a critical issue where the middleware does not actually time out if the handler is non-cooperative and blocks indefinitely. This can lead to resource exhaustion. I've added a critical review comment on this matter and a suggestion to add a test case that makes this limitation explicit.
|
@copilot review |
|
Why the grace period? If the developer wants a "grace period", they will just increase the timeout value appropriately. The grace period just adds an extra level of uncertainty where you need to look at the code to see what the value is and how it works. If it's the avoid race conditions - then it's not avoiding race conditions. It's just minimising the probability of race conditions happening. Not eliminating it. |
|
The documentation says
|
Specifically, the panic-related test cases were problematic. In scenarios where the handler panics after the timeout or where the OnTimeout handler itself panics, I observed concurrency-related issues that I couldn’t safely resolve without deeper changes. These cases were especially tricky due to goroutine lifecycle and race conditions around timeout handling. func TestTimeout_Issue_3671(t *testing.T) {
t.Parallel()
app := fiber.New()
testCases := []struct {
name string
path string
handler fiber.Handler
config Config
expectCode int
}{
{
name: "Handler panics after timeout",
path: "/panic-after-timeout",
handler: func(_ fiber.Ctx) error {
time.Sleep(50 * time.Millisecond)
panic("panic after timeout")
},
config: Config{Timeout: 10 * time.Millisecond},
expectCode: fiber.StatusRequestTimeout,
},
{
name: "Handler blocks forever",
path: "/block-forever",
handler: func(_ fiber.Ctx) error {
select {} // Block forever
},
config: Config{Timeout: 10 * time.Millisecond},
expectCode: fiber.StatusRequestTimeout,
},
{
name: "Timeout set to 1 nanosecond",
path: "/nano",
handler: func(c fiber.Ctx) error {
time.Sleep(1 * time.Millisecond)
return c.SendString("late")
},
config: Config{Timeout: 1 * time.Nanosecond},
expectCode: fiber.StatusRequestTimeout,
},
{
name: "Timeout set to a very large value",
path: "/maxint",
handler: func(c fiber.Ctx) error {
return c.SendString("ok")
},
config: Config{Timeout: 1<<63 - 1},
expectCode: fiber.StatusOK,
},
{
name: "Custom OnTimeout handler panics",
path: "/panic-ontimeout",
handler: func(_ fiber.Ctx) error {
time.Sleep(50 * time.Millisecond)
return nil
},
config: Config{
Timeout: 10 * time.Millisecond,
OnTimeout: func(_ fiber.Ctx) error {
panic("custom panic on timeout")
},
},
expectCode: fiber.StatusRequestTimeout,
},
{
name: "Custom OnTimeout",
path: "/ontimeout",
handler: func(c fiber.Ctx) error {
if err := sleepWithContext(c.Context(), 100*time.Millisecond, context.DeadlineExceeded); err != nil {
return err
}
return c.SendString("should not reach")
},
config: Config{
Timeout: 20 * time.Millisecond,
OnTimeout: func(c fiber.Ctx) error {
return c.SendString("custom timeout response")
},
},
expectCode: fiber.StatusRequestTimeout,
},
}
for _, tc := range testCases {
app.Get(tc.path, New(tc.handler, tc.config))
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, tc.path, nil)
resp, err := app.Test(req)
require.NoError(t, err)
require.Equal(t, tc.expectCode, resp.StatusCode)
})
}
} |
…-return-and-context-propagation
…-context-propagation' into fix/timeout-immediate-return-and-context-propagation
…d handler management
|
@pjebs You're right - the fixed grace period was logically inconsistent. We've changed the approach: New behavior:
This gives users full control. The default is now the safest option (always wait), and users who want faster responses can Regarding spelling: I changed the places to US @edvardsanta Thanks for testing and identifying the concurrency issues with panic scenarios! The current implementation handles panics by:
The tests TestTimeout_PanicInHandler and TestTimeout_PanicAfterTimeout verify this behavior. If you're still seeing issues with specific scenarios, please share the details and we can investigate further. |
|
I think Fiber internals needs to be rewritten a bit. FastHTTP's timeout middleware doesn't have any race conditions. |
|
I have now expanded our core and implemented a race-free approach similar to the fasthttp approach. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement to the timeout middleware by allowing it to return immediately when a timeout occurs, which is achieved by running handlers in a separate goroutine. The implementation also adds panic recovery and a new context 'abandon' mechanism to manage context lifecycle in these detached scenarios. My review focuses on a few critical aspects of this new implementation. I've identified a high-severity issue where recovered panics are not logged, making debugging difficult. More critically, the current approach introduces a memory leak for every timed-out request as a trade-off for race safety. While the safety aspect is understood, this is a significant behavioral change that needs to be addressed or at least prominently documented. I've also pointed out that the documentation for the new Abandon/ForceRelease APIs is inconsistent with their actual use in the timeout middleware.
…-return-and-context-propagation
…-context-propagation' into fix/timeout-immediate-return-and-context-propagation
|
Maybe you can unexport the Abandon method you just created, and call it using unsafe or reflect. Others don't need to know about it. The API surface and documentation is already bloated. |
its okay like it is |
ReneWerner87
deleted the
fix/timeout-immediate-return-and-context-propagation
branch
Summary
c.Context().Done()Replace #3680
Replace #3826
Changes
context.WithTimeout(unlike fix up timeout middleware #3826 which usedtime.After) so handlers can detect cancellation500 Internal Server ErrorrunHandlerandisCustomErrorinto singleisTimeoutErrorfunctiondocs/middleware/timeout.mdanddocs/whats_new.mdBreaking Changes
None - The API remains the same, but behavior is improved:
Test Plan
TestTimeout_ImmediateReturn- Verifies immediate response on timeoutTestTimeout_ContextPropagation- Verifies handler can detect context cancellationTestTimeout_PanicInHandler- Verifies panic handlingTestTimeout_PanicAfterTimeout- Verifies late panics don't affect responseCloses #3671
Closes #3394