🔥 feat: added cancelFunc and async goroutine to monitore lost of connec…#4404
🔥 feat: added cancelFunc and async goroutine to monitore lost of connec…#4404dima11223432 wants to merge 13 commits into
Conversation
…tion to canel ctx
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDefaultCtx gains a ChangesRequest-scoped context cancellation
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultCtx as DefaultCtx.Context()
participant Fasthttp as fasthttp.RequestCtx
participant Canceller as CancellationGoroutine
Client->>DefaultCtx: call Context()
DefaultCtx->>Fasthttp: read user context (user values)
alt no context
DefaultCtx->>DefaultCtx: context.WithCancel(background)
DefaultCtx->>Fasthttp: SetContext(newCtx)
end
DefaultCtx->>Canceller: spawn goroutine (if connection present)
Canceller->>Fasthttp: wait on Fasthttp.Done()
Canceller->>DefaultCtx: wait on stored ctx.Done()
alt fasthttp done first
Canceller->>DefaultCtx: call cancelFunc()
Canceller->>DefaultCtx: clear cancelFunc
else ctx done first
Canceller->>Canceller: return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 context cancellation handling in DefaultCtx by adding a cancelFunc field, managing context cancellation inside Context(), and resetting/canceling it during Reset(). However, the reviewer identified critical issues with these changes: the background goroutine and 10ms ticker introduced in Reset() cause severe performance regressions, data races, and goroutine/CPU leaks. Additionally, the background goroutine in Context() introduces a potential deadlock, goroutine leak, and data race. The reviewer recommends removing the background goroutine from Reset() entirely and provides a refactored implementation for Context() to safely manage the context lifecycle.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ctx.go`:
- Around line 717-738: The watcher goroutine in DefaultCtx.Reset reads
c.fasthttp concurrently, causing a race and potential nil deref; change the
goroutine to capture the per-request *fasthttp.RequestCtx into a local (e.g.
fctx := c.fasthttp) and use fctx.UserValue(userContextKey) instead of reading
c.fasthttp on each tick, and modify release() to call and nil c.cancelFunc
(invoke the stored cancel function and set c.cancelFunc = nil) so the ticker
goroutine is deterministically stopped when the context is returned to the pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
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)
143-155:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMultiple goroutines spawned per request and race condition on
cancelFunc.Two issues with the goroutine spawn logic:
Multiple goroutines per request: The goroutine spawn block runs on every
Context()call whenc.cancelFunc != nil. SincecancelFuncis set inReset()and never cleared after spawning, each subsequent call toContext()spawns another goroutine doing the same work.Race on cancelFunc capture: Between the check
c.cancelFunc != nil(line 143) and the capturecancel := c.cancelFunc(line 146), another goroutine could callReset()/release()settingc.cancelFunc = nil. The capturedcancelbecomes nil, causing a panic when the goroutine callscancel().🐛 Suggested fix: atomically claim cancelFunc when spawning
- if c.cancelFunc != nil && c.fasthttp.Conn() != nil { - fastHttpDone := c.fasthttp.Done() - reqCtx := ctx - cancel := c.cancelFunc - - go func() { - select { - case <-fastHttpDone: - cancel() - case <-reqCtx.Done(): - } - }() - } + // Atomically claim the cancelFunc to spawn the monitoring goroutine only once. + // Once claimed (set to nil here), subsequent Context() calls won't spawn duplicates. + if cancel := c.cancelFunc; cancel != nil && c.fasthttp.Conn() != nil { + c.cancelFunc = nil // Claim: prevent duplicate goroutines + fastHttpDone := c.fasthttp.Done() + reqCtx := ctx + + go func() { + select { + case <-fastHttpDone: + cancel() + case <-reqCtx.Done(): + } + }() + }Note: If
cancelFuncis cleared inContext(), ensureReset()andrelease()still check for nil before calling, which they already do. However, this means callingc.cancelFunc()inReset()/release()may not cancel an in-flight monitoring goroutine ifContext()already claimed it. Consider whether the goroutine'scancelshould still be reachable for explicit cleanup, or if relying onreqCtx.Done()is sufficient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctx.go` around lines 143 - 155, The monitoring goroutine in Context() can be spawned repeatedly and races on c.cancelFunc; fix by atomically claiming and clearing the cancel function before starting the goroutine so only one monitor is created and you never capture a nil cancel. Modify the Context() logic (referencing Context(), c.cancelFunc, c.fasthttp.Done()) to acquire the cancel function under the same synchronization used by Reset()/release() (or use an atomic compare-and-swap on a pointer to the cancel func) and set c.cancelFunc = nil as you claim it, then spawn the goroutine with the claimed non-nil cancel; leave Reset()/release() defensive (check for nil) as they already do.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ctx.go`:
- Around line 143-155: The monitoring goroutine in Context() can be spawned
repeatedly and races on c.cancelFunc; fix by atomically claiming and clearing
the cancel function before starting the goroutine so only one monitor is created
and you never capture a nil cancel. Modify the Context() logic (referencing
Context(), c.cancelFunc, c.fasthttp.Done()) to acquire the cancel function under
the same synchronization used by Reset()/release() (or use an atomic
compare-and-swap on a pointer to the cancel func) and set c.cancelFunc = nil as
you claim it, then spawn the goroutine with the claimed non-nil cancel; leave
Reset()/release() defensive (check for nil) as they already do.
|
@dima11223432 Please add unit-tests/docs before submitting a PR. |
|
Can i write only unit tests? :) |
|
Please test your code locally first and add unit tests to it before creating pull requests marked as "done" |
|
@ReneWerner87 I have written tests for cancelFunc! Pls, reopen pr |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ctx_test.go`:
- Around line 9601-9711: Add t.Parallel() at the start of the top-level tests
and each subtest to comply with the repository rule: insert t.Parallel() as the
first statement in Test_Ctx_Context_Lazy_Initialization_Suite, inside each t.Run
callback ("Lazy_Initialization_With_Conn", "Lazy_Initialization_Without_Conn",
"Release_Without_Context_Access"), and at the start of
Test_Ctx_Context_Cancel_On_Disconnect so the tests and subtests run in parallel.
- Around line 9680-9708: Test_Ctx_Context_Cancel_On_Disconnect is cancelling the
injected netCtx directly instead of exercising the disconnect path in
DefaultCtx.Context(); instead of calling cancelNet(), arrange for the fasthttp
Done signal to fire so the code path that checks c.cancelFunc &&
c.fasthttp.Conn() is exercised: create a fasthttp.RequestCtx (or a small test
double) whose Done() returns a cancellable channel (e.g., create a
context.WithCancel and wire its Done into the RequestCtx/Done return), assign
that RequestCtx to c.fasthttp and ensure c.cancelFunc is non-nil (via
c.SetContext or by setting cancelFunc like in production), then trigger the
RequestCtx cancellation (call the cancel returned by context.WithCancel) and
assert goCtx.Done() is closed and goCtx.Err() is context.Canceled rather than
cancelling netCtx directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c7d6b547-d1e4-4818-958d-5446b93968d4
📒 Files selected for processing (2)
ctx_test.gomiddleware/timeout/timeout_test.go
|
Tests and lint are falling |
|
I have resolved linter errors and data race |
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4404 +/- ##
==========================================
- Coverage 93.01% 92.92% -0.09%
==========================================
Files 139 139
Lines 13789 13813 +24
==========================================
+ Hits 12826 12836 +10
- Misses 596 607 +11
- Partials 367 370 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Description
This PR fixes a critical resource leak and testing issue where the background monitoring goroutine for client-disconnect detection could get stuck in a select block indefinitely.
When a middleware (like timeout) abandons the original request context or a context panics after a timeout, the fasthttp.RequestCtx might never close its connection channel in test environments.
The solution ensures the lifecycle of the connection-monitoring goroutine is strictly bound to the local request context (reqCtx.Done()), allowing it to gracefully exit the moment the context is canceled or reset by the Fiber engine.
Fixes #4338
Changes introduced
Type of change
Checklist