-
Notifications
You must be signed in to change notification settings - Fork 8.3k
add a thread safe context that can be used downstream and mutated #4172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
905cdca
to
443ef77
Compare
@rubensayshi Please update the PR to the latest source code for the CI/CD flow. |
443ef77
to
cd8ad99
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4172 +/- ##
==========================================
- Coverage 99.21% 98.75% -0.47%
==========================================
Files 42 44 +2
Lines 3182 3445 +263
==========================================
+ Hits 3157 3402 +245
- Misses 17 32 +15
- Partials 8 11 +3
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:
|
happy to increase the coverage if there's an actual chance this would get merged! :D |
can we get someone to look into it ? it's failing many of our concurrency tests because of the race condition |
I will take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces thread-safe context management capabilities to Gin by adding an internal context mechanism that allows safe mutations without race conditions on c.Request.Context()
. The implementation addresses known race detection issues when using context values in concurrent scenarios.
- Adds
UseInternalContext
feature flag to enable thread-safe internal context management - Implements
WithInternalContext()
andInternalContext()
methods with proper mutex protection - Modifies existing context methods to support internal context fallback when enabled
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
context.go |
Core implementation of thread-safe internal context with mutex protection and context interface methods |
gin.go |
Adds UseInternalContext configuration flag and updates comment formatting |
context_test.go |
Comprehensive test coverage for all internal context methods and edge cases |
test_helpers.go |
Enhances CreateTestContext with optional configuration functions for testing |
Comments suppressed due to low confidence (1)
context.go:1357
- [nitpick] The function name
useInternalContext
is ambiguous - it reads like an action rather than a predicate. Consider renaming tohasInternalContext
orisInternalContextEnabled
to better indicate it's a boolean check.
func (c *Context) useInternalContext() bool {
return c.internalContext | ||
} | ||
|
||
// hasRequestContext returns whether c.Request has Context and fallback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is incorrect for the useInternalContext
function. It should describe that this function checks if the UseInternalContext feature flag is enabled, not about request context fallback.
// hasRequestContext returns whether c.Request has Context and fallback. | |
// useInternalContext checks if the UseInternalContext feature flag is enabled. |
Copilot uses AI. Check for mistakes.
It's currently unsafe to use
c.Request.Context
from the*gin.Context
to pass to downstream functions AND do any mutations to thec.Request.Context
.Even when you do
c.Request = c.Request.WithContext(...)
you'll end up with the go race detector flagging the unsafe reading of thec.Request
and writing toc.Request
.And even when you're not really doing anything
.WithContext
there's a chance that the context pool causes the race detector to get angryHowever it would be really nice to be able to put values into the
*gin.Context
withcontext.WithValue()
in a thread-safe way that allows downstream usage of*gin.Context
ascontext.Context
to access those values.For example, an auth middleware adding some debugging /loggin related values with
context.WithValue()
will not be lost along the way, or adding aotel.Tracer
to the context with additional options etc.Making all the places where gin touches
c.Request
thread-safe with a lock would be a significant undertaking and would have an impact on performance (probably talking about nano seconds, but still non zero impact).With the MR we're adding a thread-safe way to carry around and mutate a
context.Context
inside*gin.Context
without having to protectc.Request
.In practice the
c.Request.Context
isn't going to be very interesting to wrap anyway, so loosing that as the "root" context probably isn't a issue for anyone.It's trivial to make the root
InternalContext
thec.Request.Context
when we do.reset()
(ifContextWithFallback
is enabled), but I wanted to keep this MR as small as possible.And it would have one quirky downside that if someone uses
UseInternalContext
and does something likec.Request = c.Request.WithContext(...)
then it wouldn't have any effect.There's a couple of mentions of issues around this before:
DATA RACE
with Go1.21, Go1.22 #3931func (c *Context) Copy() *Context
#1118