-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
🩹 fix: Timeout middleware not enforced #3680
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?
🩹 fix: Timeout middleware not enforced #3680
Conversation
|
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. WalkthroughThe timeout middleware refactoring enforces handler timeouts through concurrent execution with goroutine-based synchronization, panic capture, and context cancellation. The change replaces linear post-execution timeout checks with a select-based control flow that monitors completion, panics, and deadline exceeded conditions, unified through a single error path. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Handler Goroutine
participant Done Channel
participant Panic Channel
participant Context
Client->>Middleware: HTTP Request
Middleware->>Context: Create cancellable context with timeout
Middleware->>Handler Goroutine: Launch handler in goroutine
Middleware->>Middleware: Enter select statement
alt Successful Completion
Handler Goroutine->>Done Channel: Signal completion
Done Channel->>Middleware: Receive result
Middleware->>Middleware: Check for custom error
Middleware->>Client: Return response
else Timeout/Deadline Exceeded
Context->>Context: Deadline exceeded
Context->>Middleware: Context cancellation detected
Middleware->>Middleware: Invoke OnTimeout callback
Middleware->>Client: Return fiber.ErrRequestTimeout
else Panic in Handler
Handler Goroutine->>Panic Channel: Capture panic
Panic Channel->>Middleware: Receive panic signal
Middleware->>Client: Return fiber.ErrRequestTimeout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
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.
Summary of Changes
Hello @edvardsanta, 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 addresses a critical bug in the Fiber framework's timeout middleware where it failed to properly enforce timeouts, particularly when request handlers blocked indefinitely or panicked after the timeout period. The changes refactor the middleware to execute handlers concurrently, enabling reliable timeout detection and robust panic recovery for both the main handler and custom timeout callbacks. This ensures that the application consistently responds with a Request Timeout status when a handler exceeds its allocated time, significantly improving the middleware's reliability and resilience.
Highlights
- Concurrent Handler Execution: The timeout middleware now executes the wrapped handler in a separate goroutine. This allows the middleware to effectively monitor the handler's execution time independently and enforce timeouts even if the handler blocks indefinitely.
- Improved Timeout Enforcement: The middleware now uses a
selectstatement to wait for one of three events: the handler completing successfully, the handler panicking, or the context timing out. This ensures that timeouts are reliably enforced and appropriatefiber.ErrRequestTimeoutresponses are returned. - Enhanced Panic Handling: Robust panic recovery has been implemented for both the main handler and the custom
OnTimeoutcallback. If either panics, the middleware catches the panic and ensures that afiber.ErrRequestTimeoutis returned, preventing application crashes. - Expanded Test Coverage: Comprehensive new test cases have been added to cover various challenging scenarios, including handlers that block forever, handlers that panic, and situations where the custom
OnTimeoutcallback itself panics.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request correctly refactors the timeout middleware to use a goroutine and a select statement, which properly enforces timeouts. This fixes a fundamental bug in the previous implementation. The new test suite also adds valuable coverage for edge cases. My main feedback is regarding panic handling. The current implementation swallows panics from the handler and returns a timeout error, which can hide critical bugs. I've suggested re-panicking instead to let the framework's standard error handling take over.
|
Hello, @ReneWerner87, @gaby ,@own2pwn, @sixcolors. I’m exploring some edge cases around the Timeout middleware and would like your thoughts on expected behavior. Specifically, I’m wondering how Fiber should behave when a panic occurs in the handler. I have written two tests cases: handler: func(_ fiber.Ctx) error {
time.Sleep(50 * time.Millisecond)
panic("panic after timeout")
}Custom OnTimeout handler panics: 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")
},
},My questions:
I’m not sure what the “correct” behavior should be here, so I’d like to align it first before to open the pr |
|
|
@edvardsanta Any updates on this? |
I’ll get back to this later today |
- Refactored timeout handling to utilize a safe call mechanism, ensuring that panics are captured and handled gracefully. - Updated the timeout logic to return appropriate errors based on the context and configuration, enhancing clarity and reliability.
# Conflicts: # middleware/timeout/timeout.go
…Call function - OnTimeout was being called twice
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.
Pull Request Overview
This PR fixes the timeout middleware to properly enforce request timeouts in real-time by refactoring the middleware implementation to use goroutines and channels for timeout handling.
Key changes:
- Wraps handler execution in a goroutine with proper panic recovery
- Uses a
selectstatement to handle handler completion, panics, and timeout scenarios - Ensures timeouts are enforced in real-time rather than only after handler completion
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/timeout/timeout.go | Refactored timeout enforcement logic using goroutines and channels |
| middleware/timeout/timeout_test.go | Added comprehensive test cases for edge cases including panics, blocking handlers, and extreme timeout values |
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
39-50: Panics are being downgraded toErrRequestTimeout, losing stack traces and misclassifying server bugsRight now there are two layered mechanisms that turn panics into
fiber.ErrRequestTimeout:
safeCallwraps the wholeselectand converts any panic in that block (including insidecfg.OnTimeout) tofiber.ErrRequestTimeout.- In the
select, a panic in the handler goroutine is recovered, sent intopanicChan, and then treated as a timeout:case <-panicChan: return fiber.ErrRequestTimeoutConsequences:
- A genuine handler bug (panic) is surfaced to the client as a
408 Request Timeoutinstead of a500, which is misleading from an HTTP semantics standpoint.- The original panic and its stack trace never reach Fiber’s usual panic handling path, which makes production debugging significantly harder.
- This also contradicts the stated goal of “panics are safely propagated without losing stack traces”.
A minimal change that restores expected behavior for handler panics while keeping the rest of the logic intact:
@@ func runHandler(c fiber.Ctx, h fiber.Handler, cfg Config) error { - case <-panicChan: - return fiber.ErrRequestTimeout + case p := <-panicChan: + // Re-raise the panic so Fiber's global panic handling + // can log and classify it as a 500, not a 408 timeout. + panic(p)With this change:
- Handler panics before the timeout will behave like any other Fiber panic (500 + stack).
- In the “panic after timeout” scenario, the timeout will still win the race and return 408, and the later panic will be re‑raised during the handler goroutine, preserving a stack trace rather than being silently swallowed.
Separately, you may want to reconsider whether
safeCallshould be used around the entireselectblock at all: right now a panic incfg.OnTimeoutis also mapped tofiber.ErrRequestTimeout, which again hides the root cause. It may be preferable to letOnTimeoutpanics also propagate (or at least log them explicitly) instead of downgrading them to 408.Also applies to: 55-88
🧹 Nitpick comments (2)
middleware/timeout/timeout_test.go (2)
215-312: Two table entries reuse the same path, so one scenario isn’t actually exercisedIn
TestTimeout_Issue_3671, both of these cases use"/panic-ontimeout":
"Custom OnTimeout handler panics""Custom OnTimeout"Since you register routes in a loop:
for _, tc := range testCases { app.Get(tc.path, New(tc.handler, tc.config)) }the second registration for
"/panic-ontimeout"will shadow the first, and both subtests will hit the last handler only. Because both expectStatusRequestTimeout, this passes but the “Custom OnTimeout handler panics” case isn’t truly covered at the HTTP level.Suggestion: give each scenario its own unique
path(e.g."/panic-ontimeout"vs"/custom-ontimeout") so both code paths (panic inOnTimeoutvs normalOnTimeout) are actually exercised.
314-321:TestSafeCall_Paniccouples tests to the current “panic == timeout” behaviorThis test asserts that any panic wrapped by
safeCallbecomesfiber.ErrRequestTimeout:err := safeCall(func() error { panic("test panic") }) require.Equal(t, fiber.ErrRequestTimeout, err)If you adopt a model where handler panics (or
OnTimeoutpanics) are re‑raised to go through Fiber’s global panic handling (to preserve stack traces and yield 500s), this test will need to be updated accordingly—either to:
- expect a re‑panic, or
- restrict
safeCall’s use to only those places where you truly want to convert a panic into a timeout-like error.As it stands, the test locks in the “panic becomes timeout” behavior, so any change in the runtime panic semantics should be reflected here.
📜 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)
🧰 Additional context used
🧠 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.
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: 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.
📚 Learning: 2025-05-08T08:14:37.302Z
Learnt from: mdelapenya
Repo: gofiber/fiber PR: 3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Applied to files:
middleware/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.gomiddleware/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.go
📚 Learning: 2024-09-25T17:05:06.991Z
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.
Applied to files:
middleware/timeout/timeout.gomiddleware/timeout/timeout_test.go
📚 Learning: 2024-12-01T10:28:36.011Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2025-05-13T00:19:16.407Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3446
File: docs/middleware/logger.md:44-44
Timestamp: 2025-05-13T00:19:16.407Z
Learning: In documentation files for the Fiber framework, code examples are often partial and don't repeat import statements that were shown in earlier examples, focusing instead on demonstrating specific usage patterns.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-07-15T19:53:36.605Z
Learnt from: luk3skyw4lker
Repo: gofiber/fiber PR: 3074
File: router_test.go:372-397
Timestamp: 2024-07-15T19:53:36.605Z
Learning: There's no short-term plan to support concurrency on the `RebuildTree` method in the Fiber framework as per luk3skyw4lker.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2025-11-13T11:34:38.503Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3824
File: listen_test.go:560-564
Timestamp: 2025-11-13T11:34:38.503Z
Learning: The gofiber/fiber project only supports the latest Go versions, so Go 1.22+ syntax features (such as `for range N` integer range loops) are acceptable and preferred.
Applied to files:
middleware/timeout/timeout.go
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
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-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.go
🧬 Code graph analysis (2)
middleware/timeout/timeout.go (3)
constants.go (1)
ErrRequestTimeout(126-126)ctx_interface_gen.go (1)
Ctx(18-432)middleware/timeout/config.go (1)
Config(10-26)
middleware/timeout/timeout_test.go (4)
middleware/timeout/timeout.go (1)
New(14-37)middleware/timeout/config.go (1)
Config(10-26)ctx_interface_gen.go (1)
Ctx(18-432)constants.go (4)
StatusRequestTimeout(81-81)StatusOK(52-52)MethodGet(5-5)ErrRequestTimeout(126-126)
⏰ 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: repeated
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (1)
middleware/timeout/timeout.go (1)
17-37: New middleware wiring and context restoration look soundThe wrapper correctly:
- skips on
cfg.Next,- sets up a per-request timeout context when
Timeout > 0,- restores the original context and calls
cancel()viadefereven when the handler returns an error or panics (assuming we re‑panic from insiderunHandler).Calling
runHandlerin both the timeout and non-timeout branches keeps the behavior centralized and consistent.
Description
This PR refactors the timeout middleware to properly enforce request timeouts by:
Wrapping the handler execution in a goroutine.
2 Using a
selectstatement to handle three possible outcomes:This change guarantees that:
Fixes #3671
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md