🐛 bug: delegate Ctx context methods to SetContext#4399
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughDefaultCtx now forwards Deadline, Done, and Err to a stored context set via ChangesContext Delegation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates Fiber’s Ctx context.Context behavior so Deadline(), Done(), and Err() delegate to a custom context set via SetContext, and aligns documentation and tests accordingly.
Changes:
DefaultCtxnow forwardsDeadline(),Done(), andErr()to the context stored bySetContext(otherwise remains a no-op).- Documentation updated to describe the custom-context delegation behavior.
- Added a regression test verifying delegation to a custom context; updated linter suppression comment for
os.Exitusage.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| log/default.go | Adjusts revive suppression for intentional os.Exit(1) on fatal logs. |
| ctx.go | Implements delegation of Deadline/Done/Err to a custom context stored in fasthttp user values. |
| ctx_test.go | Adds a test ensuring context methods delegate when a custom context is set. |
| docs/guide/context.md | Updates guide text and summary to reflect delegation behavior. |
| docs/api/ctx.md | Updates API docs to describe SetContext delegation semantics. |
Files not reviewed (1)
- ctx_interface_gen.go: Language not supported
| bytebufferpool.Put(buf) | ||
| if lv == LevelFatal { | ||
| os.Exit(1) //nolint:revive // we want to exit the program when Fatal is called | ||
| os.Exit(1) //revive:disable-line:deep-exit // Fatal logs intentionally terminate the process. |
| bytebufferpool.Put(buf) | ||
| if lv == LevelFatal { | ||
| os.Exit(1) //nolint:revive // we want to exit the program when Fatal is called | ||
| os.Exit(1) //revive:disable-line:deep-exit // Fatal logs intentionally terminate the process. |
| bytebufferpool.Put(buf) | ||
| if lv == LevelFatal { | ||
| os.Exit(1) //nolint:revive // we want to exit the program when Fatal is called | ||
| os.Exit(1) //revive:disable-line:deep-exit // Fatal logs intentionally terminate the process. |
| func (c *DefaultCtx) Deadline() (time.Time, bool) { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Deadline() | ||
| } | ||
| } | ||
| return time.Time{}, false | ||
| } |
| func (c *DefaultCtx) Done() <-chan struct{} { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Done() | ||
| } | ||
| } | ||
| return nil | ||
| } |
| func (c *DefaultCtx) Err() error { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Err() //nolint:wrapcheck // Err mirrors the stored context. | ||
| } | ||
| } | ||
| return nil | ||
| } |
![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 updates the Deadline, Done, and Err methods of DefaultCtx to delegate to a custom context if one has been set via SetContext, rather than operating as no-ops. It also updates the corresponding interface documentation, adds unit tests, and adjusts linter directives in the logger. The review feedback suggests optimizing these context methods by checking c.isUserContextSet first, which avoids the overhead of map lookups and type assertions for the majority of requests that do not use a custom context.
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.
| func (c *DefaultCtx) Deadline() (time.Time, bool) { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Deadline() | ||
| } | ||
| } | ||
| return time.Time{}, false | ||
| } |
There was a problem hiding this comment.
Checking c.isUserContextSet before performing the UserValue lookup and type assertion is a great optimization. Since isUserContextSet is only set to true when a custom context is actually configured, we can completely bypass the overhead of the map/slice lookup and type assertion for the vast majority of requests that do not use a custom context.
| func (c *DefaultCtx) Deadline() (time.Time, bool) { | |
| if c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Deadline() | |
| } | |
| } | |
| return time.Time{}, false | |
| } | |
| func (c *DefaultCtx) Deadline() (time.Time, bool) { | |
| if c.isUserContextSet && c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Deadline() | |
| } | |
| } | |
| return time.Time{}, false | |
| } |
| func (c *DefaultCtx) Done() <-chan struct{} { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Done() | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Checking c.isUserContextSet before performing the UserValue lookup and type assertion is a great optimization. Since isUserContextSet is only set to true when a custom context is actually configured, we can completely bypass the overhead of the map/slice lookup and type assertion for the vast majority of requests that do not use a custom context.
| func (c *DefaultCtx) Done() <-chan struct{} { | |
| if c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Done() | |
| } | |
| } | |
| return nil | |
| } | |
| func (c *DefaultCtx) Done() <-chan struct{} { | |
| if c.isUserContextSet && c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Done() | |
| } | |
| } | |
| return nil | |
| } |
| func (c *DefaultCtx) Err() error { | ||
| if c.fasthttp != nil { | ||
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | ||
| return ctx.Err() //nolint:wrapcheck // Err mirrors the stored context. | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Checking c.isUserContextSet before performing the UserValue lookup and type assertion is a great optimization. Since isUserContextSet is only set to true when a custom context is actually configured, we can completely bypass the overhead of the map/slice lookup and type assertion for the vast majority of requests that do not use a custom context.
| func (c *DefaultCtx) Err() error { | |
| if c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Err() //nolint:wrapcheck // Err mirrors the stored context. | |
| } | |
| } | |
| return nil | |
| } | |
| func (c *DefaultCtx) Err() error { | |
| if c.isUserContextSet && c.fasthttp != nil { | |
| if ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context); ok && ctx != nil { | |
| return ctx.Err() //nolint:wrapcheck // Err mirrors the stored context. | |
| } | |
| } | |
| return nil | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4399 +/- ##
==========================================
+ Coverage 93.17% 93.22% +0.04%
==========================================
Files 140 140
Lines 14541 14550 +9
==========================================
+ Hits 13549 13564 +15
+ Misses 619 614 -5
+ Partials 373 372 -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:
|
|
@ReneWerner87 If I remember correctly we left these as |
|
Will check in some hours,need to make a little bit research |
Summary
DefaultCtx.Deadline,Done, andErrto the context set bySetContext, while preserving the current no-op fallback when no context is set.revivesuppressions to directrevive:disable-linedirectives so the repo lint target stays clean.Closes #4335
Test plan
go test ./ -run 'Test_Ctx_(Deadline|Done|Err|Context|ContextMethods_UseCustomContext)' -count=1make generatemake formatmake betteraligngit diff --checkmake lintmake testGOVERSION=go1.26.4 make auditmake markdown