🐛 bug: pin Context() not canceled on graceful shutdown - #4599
Conversation
Document and regression-test that c.Context() is not canceled when the server shuts down, while RequestCtx.Done() is. Closes gofiber#3431
WalkthroughThe change documents that ChangesGraceful shutdown context behavior
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
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 `@app_test.go`:
- Around line 1526-1544: Update the test around app.Listener and the client
goroutine to replace the fixed 50 ms sleep with a ListenConfig.BeforeServeFunc
readiness signal, and report both goroutine errors through channels. Wait for
listener readiness before dialing, then join both goroutines after
ShutdownWithTimeout so connection, copy, close, and listener errors are asserted
before the test exits.
🪄 Autofix
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 Plus
Run ID: 8d8db549-e837-47a6-9b63-07a7e5cce576
📒 Files selected for processing (2)
app_test.godocs/api/ctx.md
| ln := fasthttputil.NewInmemoryListener() | ||
| go func() { | ||
| assert.NoError(t, app.Listener(ln)) | ||
| }() | ||
| time.Sleep(50 * time.Millisecond) | ||
|
|
||
| go func() { | ||
| conn, err := ln.Dial() | ||
| assert.NoError(t, err) | ||
| _, err = conn.Write([]byte("GET /slow HTTP/1.1\r\nHost: example.com\r\n\r\n")) | ||
| assert.NoError(t, err) | ||
| // Drain response so the connection can finish cleanly. | ||
| _, copyErr := io.Copy(io.Discard, conn) | ||
| assert.NoError(t, copyErr) | ||
| assert.NoError(t, conn.Close()) | ||
| }() | ||
|
|
||
| <-handlerStarted | ||
| require.NoError(t, app.ShutdownWithTimeout(2*time.Second)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize and join the listener and client goroutines.
The fixed 50 ms delay does not prove that app.Listener is ready. If ln.Dial() fails, handlerStarted never closes and the test blocks indefinitely. The client goroutine is also not joined, so its assertions can run after the test returns.
Use a listener-ready signal, such as ListenConfig.BeforeServeFunc, and return listener and client errors through channels. Wait for both goroutines before the test exits.
🤖 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 `@app_test.go` around lines 1526 - 1544, Update the test around app.Listener
and the client goroutine to replace the fixed 50 ms sleep with a
ListenConfig.BeforeServeFunc readiness signal, and report both goroutine errors
through channels. Wait for listener readiness before dialing, then join both
goroutines after ShutdownWithTimeout so connection, copy, close, and listener
errors are asserted before the test exits.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4599 +/- ##
==========================================
- Coverage 93.47% 93.43% -0.05%
==========================================
Files 140 140
Lines 14983 14983
==========================================
- Hits 14006 14000 -6
- Misses 608 613 +5
- Partials 369 370 +1
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:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff734065d
ℹ️ 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
- 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 address that feedback".
| select { | ||
| case <-c.Context().Done(): | ||
| contextErr <- c.Context().Err() | ||
| case <-time.After(500 * time.Millisecond): |
There was a problem hiding this comment.
If shutdown reaches fasthttp between the 500 ms timeout here and the subsequent RequestCtx.Done() check, both assertions pass even if c.Context() is incorrectly canceled at exactly the same time as RequestCtx. This can produce a false-positive regression test under scheduling delays; wait for RequestCtx.Done() as the shutdown-start barrier first, then check that c.Context() remains uncanceled.
Useful? React with 👍 / 👎.
Summary
Pins and documents the v3 behavior for #3431:
c.Context()is not canceled when graceful shutdown starts (default iscontext.Background()unless youSetContext). By contrast,c.RequestCtx().Done()is closed when fasthttp begins shutdown.In-flight handlers that pass
c.Context()to DB/clients can finish during graceful shutdown.Changes
ShutdownWithTimeoutContext()vsRequestCtxLinked issue
Closes #3431
Checklist
make formatmake lint— 0 issues