🔥 feat: added cancelFunc and async goroutine to monitore lost of connec…#4404
Open
dima11223432 wants to merge 11 commits into
Open
🔥 feat: added cancelFunc and async goroutine to monitore lost of connec…#4404dima11223432 wants to merge 11 commits into
dima11223432 wants to merge 11 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 |
Contributor
|
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}
Contributor
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}
Contributor
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
Contributor
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.
Member
|
@dima11223432 Please add unit-tests/docs before submitting a PR. |
Author
|
Can i write only unit tests? :) |
Member
|
Please test your code locally first and add unit tests to it before creating pull requests marked as "done" |
Author
|
@ReneWerner87 I have written tests for cancelFunc! Pls, reopen pr |
Contributor
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
Comment on lines
+9601
to
+9711
| func Test_Ctx_Context_Lazy_Initialization_Suite(t *testing.T) { | ||
| app := New() | ||
|
|
||
| // Scenario with fake network connection | ||
| t.Run("Lazy_Initialization_With_Conn", func(t *testing.T) { | ||
| fctx := &fasthttp.RequestCtx{} | ||
| remoteAddr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8080} | ||
| fctx.Init(&fasthttp.Request{}, remoteAddr, nil) | ||
|
|
||
| ctx := app.AcquireCtx(fctx) | ||
| c, ok := ctx.(*DefaultCtx) | ||
| if !ok { | ||
| t.Fatal("AcquireCtx did not return *DefaultCtx") | ||
| } | ||
|
|
||
| if c.cancelFunc == nil { | ||
| t.Fatal("expected cancelFunc to be initialized") | ||
| } | ||
|
|
||
| goCtx := c.Context() | ||
| if goCtx == nil { | ||
| t.Fatal("expected Go context to be returned") | ||
| } | ||
| if c.cancelFunc != nil { | ||
| t.Error("expected cancelFunc to be nil after lazy initialization") | ||
| } | ||
|
|
||
| goCtx2 := c.Context() | ||
| if goCtx2 != goCtx { | ||
| t.Error("expected the same context instance on sequential calls") | ||
| } | ||
|
|
||
| app.ReleaseCtx(c) | ||
| }) | ||
| //Scenario without fake network connection | ||
| t.Run("Lazy_Initialization_Without_Conn", func(t *testing.T) { | ||
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | ||
| c, ok := ctx.(*DefaultCtx) | ||
| if !ok { | ||
| t.Fatal("AcquireCtx did not return *DefaultCtx") | ||
| } | ||
|
|
||
| if c.cancelFunc == nil { | ||
| t.Fatal("expected cancelFunc to be initialized even without connection") | ||
| } | ||
|
|
||
| _ = c.Context() | ||
|
|
||
| if c.cancelFunc == nil { | ||
| t.Error("expected cancelFunc to remain non-nil when c.fasthttp.Conn() is nil") | ||
| } | ||
|
|
||
| app.ReleaseCtx(c) | ||
| }) | ||
|
|
||
| //Scenario without .Context() | ||
| t.Run("Release_Without_Context_Access", func(t *testing.T) { | ||
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | ||
| c, ok := ctx.(*DefaultCtx) | ||
| if !ok { | ||
| t.Fatal("AcquireCtx did not return *DefaultCtx") | ||
| } | ||
|
|
||
| if c.cancelFunc == nil { | ||
| t.Fatal("expected cancelFunc to be initialized") | ||
| } | ||
|
|
||
| app.ReleaseCtx(c) | ||
|
|
||
| if c.cancelFunc != nil { | ||
| t.Error("expected cancelFunc to be cleared by release()") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // go test -v -run=Test_Ctx_Context_Cancel_On_Disconnect | ||
| func Test_Ctx_Context_Cancel_On_Disconnect(t *testing.T) { | ||
| app := New() | ||
| fctx := &fasthttp.RequestCtx{} | ||
| netCtx, cancelNet := context.WithCancel(context.Background()) | ||
|
|
||
| ctx := app.AcquireCtx(fctx) | ||
| c, ok := ctx.(*DefaultCtx) | ||
| if !ok { | ||
| t.Fatal("AcquireCtx did not return *DefaultCtx") | ||
| } | ||
|
|
||
| c.SetContext(netCtx) | ||
|
|
||
| goCtx := c.Context() | ||
|
|
||
| select { | ||
| case <-goCtx.Done(): | ||
| t.Fatal("context should not be canceled yet") | ||
| default: | ||
| } | ||
|
|
||
| //close net connection | ||
| cancelNet() | ||
|
|
||
| select { | ||
| case <-goCtx.Done(): | ||
| if !errors.Is(goCtx.Err(), context.Canceled) { | ||
| t.Errorf("expected context.Canceled, got %v", goCtx.Err()) | ||
| } | ||
| case <-time.After(200 * time.Millisecond): | ||
| t.Fatal("expected Go context to be canceled after connection close") | ||
| } | ||
|
|
||
| app.ReleaseCtx(c) | ||
| } |
Contributor
There was a problem hiding this comment.
Add t.Parallel() to the new tests/subtests.
The newly added tests/subtests are missing t.Parallel(), so they do not follow this repo’s Go test rule.
♻️ Proposed fix
func Test_Ctx_Context_Lazy_Initialization_Suite(t *testing.T) {
+ t.Parallel()
app := New()
// Scenario with fake network connection
t.Run("Lazy_Initialization_With_Conn", func(t *testing.T) {
+ t.Parallel()
fctx := &fasthttp.RequestCtx{}
remoteAddr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8080}
fctx.Init(&fasthttp.Request{}, remoteAddr, nil)
@@
//Scenario without fake network connection
t.Run("Lazy_Initialization_Without_Conn", func(t *testing.T) {
+ t.Parallel()
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
@@
//Scenario without .Context()
t.Run("Release_Without_Context_Access", func(t *testing.T) {
+ t.Parallel()
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
@@
func Test_Ctx_Context_Cancel_On_Disconnect(t *testing.T) {
+ t.Parallel()
app := New()As per coding guidelines, **/*_test.go: "Always invoke t.Parallel() at the start of each test and subtest to maximize concurrency in Go tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func Test_Ctx_Context_Lazy_Initialization_Suite(t *testing.T) { | |
| app := New() | |
| // Scenario with fake network connection | |
| t.Run("Lazy_Initialization_With_Conn", func(t *testing.T) { | |
| fctx := &fasthttp.RequestCtx{} | |
| remoteAddr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8080} | |
| fctx.Init(&fasthttp.Request{}, remoteAddr, nil) | |
| ctx := app.AcquireCtx(fctx) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized") | |
| } | |
| goCtx := c.Context() | |
| if goCtx == nil { | |
| t.Fatal("expected Go context to be returned") | |
| } | |
| if c.cancelFunc != nil { | |
| t.Error("expected cancelFunc to be nil after lazy initialization") | |
| } | |
| goCtx2 := c.Context() | |
| if goCtx2 != goCtx { | |
| t.Error("expected the same context instance on sequential calls") | |
| } | |
| app.ReleaseCtx(c) | |
| }) | |
| //Scenario without fake network connection | |
| t.Run("Lazy_Initialization_Without_Conn", func(t *testing.T) { | |
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized even without connection") | |
| } | |
| _ = c.Context() | |
| if c.cancelFunc == nil { | |
| t.Error("expected cancelFunc to remain non-nil when c.fasthttp.Conn() is nil") | |
| } | |
| app.ReleaseCtx(c) | |
| }) | |
| //Scenario without .Context() | |
| t.Run("Release_Without_Context_Access", func(t *testing.T) { | |
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized") | |
| } | |
| app.ReleaseCtx(c) | |
| if c.cancelFunc != nil { | |
| t.Error("expected cancelFunc to be cleared by release()") | |
| } | |
| }) | |
| } | |
| // go test -v -run=Test_Ctx_Context_Cancel_On_Disconnect | |
| func Test_Ctx_Context_Cancel_On_Disconnect(t *testing.T) { | |
| app := New() | |
| fctx := &fasthttp.RequestCtx{} | |
| netCtx, cancelNet := context.WithCancel(context.Background()) | |
| ctx := app.AcquireCtx(fctx) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| c.SetContext(netCtx) | |
| goCtx := c.Context() | |
| select { | |
| case <-goCtx.Done(): | |
| t.Fatal("context should not be canceled yet") | |
| default: | |
| } | |
| //close net connection | |
| cancelNet() | |
| select { | |
| case <-goCtx.Done(): | |
| if !errors.Is(goCtx.Err(), context.Canceled) { | |
| t.Errorf("expected context.Canceled, got %v", goCtx.Err()) | |
| } | |
| case <-time.After(200 * time.Millisecond): | |
| t.Fatal("expected Go context to be canceled after connection close") | |
| } | |
| app.ReleaseCtx(c) | |
| } | |
| func Test_Ctx_Context_Lazy_Initialization_Suite(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| // Scenario with fake network connection | |
| t.Run("Lazy_Initialization_With_Conn", func(t *testing.T) { | |
| t.Parallel() | |
| fctx := &fasthttp.RequestCtx{} | |
| remoteAddr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8080} | |
| fctx.Init(&fasthttp.Request{}, remoteAddr, nil) | |
| ctx := app.AcquireCtx(fctx) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized") | |
| } | |
| goCtx := c.Context() | |
| if goCtx == nil { | |
| t.Fatal("expected Go context to be returned") | |
| } | |
| if c.cancelFunc != nil { | |
| t.Error("expected cancelFunc to be nil after lazy initialization") | |
| } | |
| goCtx2 := c.Context() | |
| if goCtx2 != goCtx { | |
| t.Error("expected the same context instance on sequential calls") | |
| } | |
| app.ReleaseCtx(c) | |
| }) | |
| //Scenario without fake network connection | |
| t.Run("Lazy_Initialization_Without_Conn", func(t *testing.T) { | |
| t.Parallel() | |
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized even without connection") | |
| } | |
| _ = c.Context() | |
| if c.cancelFunc == nil { | |
| t.Error("expected cancelFunc to remain non-nil when c.fasthttp.Conn() is nil") | |
| } | |
| app.ReleaseCtx(c) | |
| }) | |
| //Scenario without .Context() | |
| t.Run("Release_Without_Context_Access", func(t *testing.T) { | |
| t.Parallel() | |
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| if c.cancelFunc == nil { | |
| t.Fatal("expected cancelFunc to be initialized") | |
| } | |
| app.ReleaseCtx(c) | |
| if c.cancelFunc != nil { | |
| t.Error("expected cancelFunc to be cleared by release()") | |
| } | |
| }) | |
| } | |
| // go test -v -run=Test_Ctx_Context_Cancel_On_Disconnect | |
| func Test_Ctx_Context_Cancel_On_Disconnect(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| fctx := &fasthttp.RequestCtx{} | |
| netCtx, cancelNet := context.WithCancel(context.Background()) | |
| ctx := app.AcquireCtx(fctx) | |
| c, ok := ctx.(*DefaultCtx) | |
| if !ok { | |
| t.Fatal("AcquireCtx did not return *DefaultCtx") | |
| } | |
| c.SetContext(netCtx) | |
| goCtx := c.Context() | |
| select { | |
| case <-goCtx.Done(): | |
| t.Fatal("context should not be canceled yet") | |
| default: | |
| } | |
| //close net connection | |
| cancelNet() | |
| select { | |
| case <-goCtx.Done(): | |
| if !errors.Is(goCtx.Err(), context.Canceled) { | |
| t.Errorf("expected context.Canceled, got %v", goCtx.Err()) | |
| } | |
| case <-time.After(200 * time.Millisecond): | |
| t.Fatal("expected Go context to be canceled after connection close") | |
| } | |
| app.ReleaseCtx(c) | |
| } |
🤖 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_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+9680
to
+9708
| netCtx, cancelNet := context.WithCancel(context.Background()) | ||
|
|
||
| ctx := app.AcquireCtx(fctx) | ||
| c, ok := ctx.(*DefaultCtx) | ||
| if !ok { | ||
| t.Fatal("AcquireCtx did not return *DefaultCtx") | ||
| } | ||
|
|
||
| c.SetContext(netCtx) | ||
|
|
||
| goCtx := c.Context() | ||
|
|
||
| select { | ||
| case <-goCtx.Done(): | ||
| t.Fatal("context should not be canceled yet") | ||
| default: | ||
| } | ||
|
|
||
| //close net connection | ||
| cancelNet() | ||
|
|
||
| select { | ||
| case <-goCtx.Done(): | ||
| if !errors.Is(goCtx.Err(), context.Canceled) { | ||
| t.Errorf("expected context.Canceled, got %v", goCtx.Err()) | ||
| } | ||
| case <-time.After(200 * time.Millisecond): | ||
| t.Fatal("expected Go context to be canceled after connection close") | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant test (with line numbers)
sed -n '9600,9740p' ctx_test.go | cat -n
# Find the test definition (in case it spans more than the snippet)
rg -n "Test_Ctx_Context_Cancel_On_Disconnect" -S .
# Look for any disconnect / Done / cancellation plumbing in ctx implementation
# (best-effort: locate DefaultCtx.Context / SetContext / disconnect monitor code)
rg -n "type DefaultCtx|func \\(.*DefaultCtx.*\\) Context\\(|SetContext\\(|AcquireCtx\\(" -S . | head -n 50
rg -n "disconnect|Disconnect|fasthttp\\.Done|Done\\(\\)|onDisconnect|net\\." -S . | head -n 80
# Search for other tests asserting disconnect-driven cancellation for comparison
rg -n "Cancel_On_Disconnect|disconnect.*cancel|fasthttp.*cancel|Done.*context" -S .Repository: gofiber/fiber
Length of output: 16141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact test function (with more surrounding lines for context)
sed -n '9660,9735p' ctx_test.go | cat -n
# Inspect DefaultCtx SetContext / Context / cancellation plumbing
rg -n "func \\(c \\*DefaultCtx\\) (SetContext|Context)\\(" ctx.go
rg -n "cancelFunc|handlerDone|ScheduleReclaim|fasthttp\\.Done|client disconnect|disconnect" ctx.go
# Print relevant sections around those hits (keep it bounded)
python3 - <<'PY'
import re, subprocess, os, textwrap, sys, json, pathlib
# Find line numbers for key functions in ctx.go
targets = ["func (c *DefaultCtx) SetContext(", "func (c *DefaultCtx) Context(", "ScheduleReclaim", "cancelFunc"]
for t in targets:
out = subprocess.check_output(["bash","-lc", f"rg -n \"{re.escape(t)}\" ctx.go | head -n 5"], text=True)
if out.strip():
print(f"\n# HIT: {t}\n{out.strip()}")
PY
# Dump the likely surrounding code blocks in ctx.go for manual review
# (use sed with line ranges after locating exact line numbers)
lines=$(rg -n "func \\(c \\*DefaultCtx\\) (SetContext|Context)\\(" ctx.go | awk -F: '{print $1}' | sort -n | uniq)
for L in $lines; do
start=$((L-25)); if [ $start -lt 1 ]; then start=1; fi
end=$((L+120))
echo -e "\n### ctx.go around line $L"
sed -n "${start},${end}p" ctx.go | cat -n
done
# Also inspect ScheduleReclaim (where canceling is likely wired)
sch=$(rg -n "func \\(c \\*DefaultCtx\\) ScheduleReclaim\\(" ctx.go | awk -F: '{print $1}' | head -n 1)
if [ -n "$sch" ]; then
L=$(echo "$sch" | awk -F: '{print $1}')
start=$((L-40)); if [ $start -lt 1 ]; then start=1; fi
end=$((L+120))
echo -e "\n### ctx.go around ScheduleReclaim at line $L"
sed -n "${start},${end}p" ctx.go | cat -n
fi
# Search in request lifecycle / listener code for disconnect-driven cancellation
rg -n "AcquireCtx\\(|ReleaseCtx\\(|handlerDone|ScheduleReclaim|cancel\\(" -S . | head -n 80Repository: gofiber/fiber
Length of output: 27653
Test_Ctx_Context_Cancel_On_Disconnect is cancelling the injected context directly, not disconnect
- The test sets
netCtxviac.SetContext(netCtx), andc.Context()returns that context; callingcancelNet()directly cancelsgoCtx.Done(). - The disconnect-driven cancellation in
DefaultCtx.Context()(cancelling onc.fasthttp.Done()whenc.cancelFunc != nil && c.fasthttp.Conn() != nil) is not exercised with the emptyfasthttp.RequestCtx{}used here.
Drive cancellation through the transport/disconnect path (or trigger the fasthttp.Done() signal) and assert goCtx.Done() without directly cancelling netCtx.
🤖 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_test.go` around lines 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.
Member
|
Tests and lint are falling |
Author
|
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 92.02% 91.95% -0.07%
==========================================
Files 138 138
Lines 13486 13509 +23
==========================================
+ Hits 12410 12422 +12
- Misses 682 691 +9
- Partials 394 396 +2
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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