🐛 bug: Fix support for context.Context #3720
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. WalkthroughAdds per-request stdlib context support to Ctx via fasthttp UserValue with new Context() and SetContext() methods; clears stored context on Reset. Updates Ctx interface, docs, tests, and modifies timeout middleware to derive, set, and restore timeout-bound contexts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Fiber
participant TimeoutMW as Timeout Middleware
participant Handler as User Handler
participant Ctx as fiber.Ctx
participant StdCtx as std context
Client->>Fiber: HTTP request
Fiber->>TimeoutMW: enter middleware
TimeoutMW->>Ctx: parent := Ctx.Context()
TimeoutMW->>StdCtx: tCtx, cancel := context.WithTimeout(parent, duration)
TimeoutMW->>Ctx: Ctx.SetContext(tCtx)
TimeoutMW->>Handler: call next (handler)
Handler-->>TimeoutMW: return (may spawn goroutines using Ctx.Context())
TimeoutMW->>StdCtx: check tCtx.Err()
alt timeout occurred
TimeoutMW-->>Fiber: invoke OnTimeout (propagate error or ErrRequestTimeout)
else completed
TimeoutMW-->>Fiber: forward handler result
end
TimeoutMW->>StdCtx: cancel()
TimeoutMW->>Ctx: Ctx.SetContext(parent) %% restore parent context
Fiber-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3720 +/- ##
==========================================
- Coverage 92.08% 92.02% -0.06%
==========================================
Files 115 115
Lines 11535 11550 +15
==========================================
+ Hits 10622 10629 +7
- Misses 662 668 +6
- Partials 251 253 +2
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:
|
gaby
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for obtaining a standalone context.Context from Fiber's Ctx via new Context() and SetContext() methods. This addresses the issue that fiber.Ctx is recycled after each request and cannot be safely used in asynchronous operations that outlive the handler.
Key changes:
- Adds
Context()andSetContext()methods tofiber.Ctxfor safe async context handling - Updates timeout middleware to use the new context methods for proper timeout propagation
- Adds comprehensive documentation and tests for the new functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ctx.go |
Implements the new Context() and SetContext() methods with proper context storage and reset |
ctx_interface_gen.go |
Adds the new context methods to the Ctx interface |
ctx_test.go |
Adds comprehensive tests for context handling, including panic behavior and multi-request scenarios |
middleware/timeout/timeout.go |
Updates timeout middleware to use new context methods for proper timeout context propagation |
middleware/timeout/timeout_test.go |
Updates tests to use c.Context() and adds timing assertions |
docs/api/ctx.md |
Documents the new Context() and SetContext() methods with examples |
docs/guide/context.md |
Adds guidance on using context outside handlers and async work patterns |
docs/whats_new.md |
Updates changelog with new methods and migration guidance |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ctx.go (1)
398-402: Avoid nil-pointer panic in Value() after handler returnAccessing c.Value after return will NPE today. Fail fast with a clear message. This directly addresses the panic reported in #3718.
func (c *DefaultCtx) Value(key any) any { - return c.fasthttp.UserValue(key) + if c.fasthttp == nil { + panic("fiber: Ctx.Value() called after handler returned; do not use Ctx outside handler") + } + return c.fasthttp.UserValue(key) }
🧹 Nitpick comments (13)
ctx_interface_gen.go (2)
23-26: Fix misleading RequestCtx commentfasthttp.RequestCtx doesn’t “carry a deadline/cancellation signal” like context.Context. Reword to “returns the underlying *fasthttp.RequestCtx”.
- // RequestCtx returns *fasthttp.RequestCtx that carries a deadline - // a cancellation signal, and other values across API boundaries. + // RequestCtx returns the underlying *fasthttp.RequestCtx.
26-31: Document lifecycle constraint and panic semantics for Context()/SetContext()Per PR goal, Context() should not be callable after the handler returns. Reflect this in interface docs.
- // Context returns a context implementation that was set by - // user earlier or returns a non-nil, empty context, if it was not set earlier. + // Context returns the per-request base context previously set via SetContext, + // or a non-nil empty context if none was set. + // Panics if called after the handler returns (Ctx is recycled). - // SetContext sets a context implementation by user. + // SetContext sets the base context returned by Context(). Call only during + // the handler's lifetime.ctx.go (2)
461-461: Nit: ensure removal vs niling of user contextIf fasthttp exposes a RemoveUserValue, prefer it to avoid leaving a nil entry; otherwise this is fine.
34-36: Consider removing DefaultCtx’s context.Context implementation
The compile-time assertion_ context.Context = (*DefaultCtx)(nil)still allows passing Ctx tocontext.With*, but our scan found no direct misuse (onlycontext.WithTimeout(c.ctx, …)). Dropping this assertion would discourage future misuse while explicit panics already guard incorrect calls.docs/whats_new.md (1)
485-487: Clarify safe usage and panic behavior for Context helpersAdd an explicit caution that Ctx is recycled and that calling Context() after return panics; recommend capturing ctx inside the handler for async work.
- - **Context()**: Returns a `context.Context` that can be used outside the handler. - - **SetContext**: Sets the base `context.Context` returned by `Context()` for propagating deadlines or values. + - **Context()**: Returns a `context.Context` you can capture inside the handler and safely use outside it. + - **SetContext**: Sets the base `context.Context` returned by `Context()` for propagating deadlines or values. + - Caution: `fiber.Ctx` is recycled. Calling `c.Context()` after the handler returns panics. Always call `ctx := c.Context()` inside the handler and pass `ctx` to async work.middleware/timeout/timeout_test.go (1)
71-77: Deflake timing assertion150ms margin on a 100ms timeout can be flaky under CI load and with t.Parallel. Bump headroom or drop Parallel for this test.
- require.Less(t, elapsed, 150*time.Millisecond, "context did not cancel within timeout") + require.Less(t, elapsed, 250*time.Millisecond, "context did not cancel within timeout")Alternatively, remove t.Parallel() for this case.
docs/api/ctx.md (3)
45-52: Tighten wording; add explicit async guidance
- Fix spelling (“cancellation”).
- Explicitly state Ctx is recycled; do not use it after handler; call Context() inside handler.
-`Context` implements `context.Context`. However due to [current limitations in how fasthttp](...) works, `Deadline()`, `Done()` and `Err()` operate as a nop. The `fiber.Ctx` instance is reused after the handler returns and must not be used for asynchronous operations once the handler has completed. Call [`Context()`](#context-1) within the handler to obtain a `context.Context` that can be used outside the handler. +`Context` implements `context.Context`. However, due to [current fasthttp limitations](...), `Deadline()`, `Done()` and `Err()` are no-ops. +Important: `fiber.Ctx` is recycled. Do not use it for asynchronous work after the handler returns. +Call [`Context()`](#context-1) inside the handler to obtain a `context.Context` you can safely use outside the handler.
76-111: Document panic-on-after-return and improve examples
- Note that Context() panics if called after handler return.
- Use a typed key in SetContext example to follow context best practices.
-Returns a `context.Context` that was previously set with [`SetContext`](#setcontext). -If no context was set, it returns `context.Background()`. Unlike `fiber.Ctx` itself, -the returned context is safe to use after the handler completes. +Returns the per-request `context.Context` previously set with [`SetContext`](#setcontext). +If none was set, returns `context.Background()`. Unlike `fiber.Ctx` itself, the returned +context is safe to use after the handler completes. +Panics if `Context()` is invoked after the handler has returned. ... -app.Get("/", func(c fiber.Ctx) error { - c.SetContext(context.WithValue(context.Background(), "user", "alice")) +type userKey struct{} +app.Get("/", func(c fiber.Ctx) error { + base := context.WithValue(context.Background(), userKey{}, "alice") + c.SetContext(base) ctx := c.Context() go doWork(ctx) return nil })
333-339: Reword RequestCtx description
*fasthttp.RequestCtxis not acontext.Context; avoid implying compatibility.-Returns *fasthttp.RequestCtx that is compatible with the `context.Context` interface that requires a deadline, a cancellation signal, and other values across API boundaries. +Returns the underlying *fasthttp.RequestCtx.docs/guide/context.md (2)
58-68: Small copy edit for clarity and consistencyUse method parentheses and “retrieving” instead of “requesting.”
Apply this diff:
-You can customize the base context by calling `c.SetContext` before -requesting it: +You can customize the base context by calling `c.SetContext()` before +retrieving it:
228-230: Reinforce the “call inside handler” requirement in SummaryTie the safety note back in the Summary to reduce foot-guns.
Apply this diff:
-- Use `c.Context()` to obtain a `context.Context` that can outlive the handler, - and `c.SetContext()` to customize it with additional values or deadlines. +- Use `c.Context()` to obtain a `context.Context` that can outlive the handler + (call it inside the handler; calling it after the handler returns will panic), + and `c.SetContext()` to customize it with additional values or deadlines.ctx_test.go (2)
2849-2869: Release acquired ctx and tighten the equality assertion (minor)
- Avoid leaking pooled ctx in tests by releasing it.
- Optional: put expected first in require.Equal for clearer failure messages.
Apply this diff:
func Test_Ctx_Context(t *testing.T) { t.Parallel() app := New() c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) t.Run("Nil_Context", func(t *testing.T) { t.Parallel() ctx := c.Context() - require.Equal(t, ctx, context.Background()) + require.Equal(t, context.Background(), ctx) })
2902-2914: Release acquired ctx (minor)Free pooled ctx in this test as well.
Apply this diff:
func Test_Ctx_SetContext(t *testing.T) { t.Parallel() app := New() c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) testKey := struct{}{} testValue := "Test Value" ctx := context.WithValue(context.Background(), testKey, testValue) //nolint:staticcheck // not needed for tests c.SetContext(ctx) require.Equal(t, testValue, c.Context().Value(testKey)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
ctx.go(2 hunks)ctx_interface_gen.go(2 hunks)ctx_test.go(2 hunks)docs/api/ctx.md(2 hunks)docs/guide/context.md(3 hunks)docs/whats_new.md(2 hunks)middleware/timeout/timeout.go(2 hunks)middleware/timeout/timeout_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.mddocs/whats_new.mddocs/guide/context.md
🧠 Learnings (6)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#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`.
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#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:
ctx.goctx_interface_gen.godocs/api/ctx.mddocs/whats_new.mddocs/guide/context.md
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#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:
ctx.gomiddleware/timeout/timeout_test.goctx_interface_gen.godocs/api/ctx.mddocs/whats_new.mddocs/guide/context.md
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#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.goctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
PR: gofiber/fiber#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
PR: gofiber/fiber#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.godocs/guide/context.md
🧬 Code graph analysis (2)
middleware/timeout/timeout_test.go (2)
middleware/timeout/config.go (1)
Config(10-26)constants.go (1)
StatusRequestTimeout(81-81)
ctx_test.go (3)
middleware/timeout/timeout.go (1)
New(14-45)app.go (1)
New(515-628)ctx_interface_gen.go (1)
Ctx(18-420)
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~45-~45: There might be a mistake here.
Context: ...e handler to obtain a context.Context that can be used outside the handler. ```go...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...ly set with SetContext. If no context was set, it returns `conte...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...y Context(). Use this to propagate deadlines, cancelation signals...
(QB_NEW_EN)
docs/whats_new.md
[grammar] ~485-~485: There might be a mistake here.
Context: ...tthat can be used outside the handler. - **SetContext**: Sets the basecontext.Con...
(QB_NEW_EN)
docs/guide/context.md
[grammar] ~28-~28: There might be a mistake here.
Context: ...tain a context.Context that can safely be used after the handler returns. By de...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...context by calling c.SetContext before requesting it: ```go app.Get("/job", fu...
(QB_NEW_EN)
🔇 Additional comments (8)
middleware/timeout/timeout_test.go (3)
45-49: LGTM: use c.Context() to drive cancellationUsing c.Context() here correctly exercises the middleware-provided timeout context.
64-69: LGTM: switch to c.Context() under timeoutThis aligns tests with the new API.
158-167: LGTM: custom OnTimeout pathConfirms custom handler path with context-driven cancel.
docs/guide/context.md (1)
44-56: Async example LGTMClear and correct guidance to capture c.Context() within the handler and use it in a goroutine. Nice.
middleware/timeout/timeout.go (2)
10-13: Docstring update reads wellAccurately describes that the middleware replaces the request context with a deadline-bound one exposed via c.Context(). Good.
27-33: Context wrapping and restoration LGTMCorrectly derives tCtx from ctx.Context(), sets it for the handler, and restores the parent with cancel() in a defer. This ensures resource cleanup and proper stacking with other middleware that might also call SetContext.
ctx_test.go (2)
2870-2885: Panic-after-handler test LGTMCorrectly asserts that accessing Locals after handler completion panics.
2886-2901: Panic on c.Context() after handler LGTMMatches the documented/desired lifecycle constraint.
|
/gemini review |
![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 introduces a crucial fix for context.Context support in Fiber, allowing for safe use of contexts in asynchronous operations. The new Context() and SetContext() helpers, along with proper context recycling, address a long-standing issue. The changes are well-implemented, with thorough tests and clear documentation updates. The refactoring of the timeout middleware to use these new helpers is a great showcase of their utility. My review includes a few minor suggestions to improve the new tests by ensuring resource cleanup and removing redundant code.
|
old PR #3382 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 62e429c | Previous: e83a762 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
37-43: Double-callback fixed: OnTimeout invoked only once on deadline pathsThe outer check now triggers only when err == nil, preventing a second OnTimeout after runHandler handled a timeout error.
🧹 Nitpick comments (2)
middleware/timeout/timeout.go (2)
10-13: Doc wording drifts from behavior—clarify when ErrRequestTimeout is returnedAs implemented, ErrRequestTimeout is returned when the handler returns a deadline/custom timeout error, or when the handler returns nil but the deadline elapsed. If the handler returns a non-timeout error while the deadline also elapsed, that handler error is propagated. Reflect this to avoid confusion.
-// New enforces a timeout for each incoming request. It replaces the request's -// context with one that has the configured deadline, which is exposed through -// c.Context(). If the timeout expires or any of the specified errors occur, -// fiber.ErrRequestTimeout is returned. +// New enforces a timeout for each incoming request by replacing the request's +// context with a deadline-bound context, exposed via c.Context() for the duration +// of handler execution. If the handler returns context.DeadlineExceeded (or one +// of cfg.Errors), OnTimeout is invoked (if set) and fiber.ErrRequestTimeout is +// returned. If the handler returns nil but the deadline elapsed, OnTimeout is +// also invoked and fiber.ErrRequestTimeout is returned. Otherwise, the handler's +// error (if any) is propagated.
37-37: Nit: compare tCtx.Err() directly to the sentineltCtx.Err() returns a sentinel; direct equality is simpler and marginally faster than errors.Is here.
- if errors.Is(tCtx.Err(), context.DeadlineExceeded) && err == nil { + if tCtx.Err() == context.DeadlineExceeded && err == nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ctx.go(3 hunks)middleware/timeout/timeout.go(3 hunks)middleware/timeout/timeout_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ctx.go
- middleware/timeout/timeout_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/timeout/timeout.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#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`.
⏰ 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). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/timeout/timeout.go (2)
53-55: Propagating non-nil OnTimeout error is correctGood: allows custom OnTimeout to override the default 408 with a specific error/response.
27-33: Verified: no legacy fasthttp Context() calls remain
Allc.Context()invocations only use the stdlibcontext.ContextAPI (e.g..Value), and all fasthttp access now goes throughc.RequestCtx(). No further changes required.
ReneWerner87
deleted the
document-context.context-limitations-and-add-support
branch
Summary
context.Contextfrom Fiber'sCtxvia newContext()andSetContexthelpersfiber.Ctxis recycled and show how to use the new context for async workCtx.Context()panics if invoked after the handler and add unit tests covering context reuseFixes #3718