Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubensayshi
Copy link

@rubensayshi rubensayshi commented Mar 3, 2025

It's currently unsafe to use c.Request.Context from the *gin.Context to pass to downstream functions AND do any mutations to the c.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 the c.Request and writing to c.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 angry

However it would be really nice to be able to put values into the *gin.Context with context.WithValue() in a thread-safe way that allows downstream usage of *gin.Context as context.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 a otel.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 protect c.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 the c.Request.Context when we do .reset() (if ContextWithFallback 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 like c.Request = c.Request.WithContext(...) then it wouldn't have any effect.

There's a couple of mentions of issues around this before:

@rubensayshi rubensayshi changed the title add a thread safe context that can be used add a thread safe context that can be used downstream and mutated Mar 4, 2025
@appleboy
Copy link
Member

appleboy commented Mar 23, 2025

@rubensayshi Please update the PR to the latest source code for the CI/CD flow.

Copy link

codecov bot commented Apr 14, 2025

Codecov Report

Attention: Patch coverage is 83.92857% with 9 lines in your changes missing coverage. Please review.

Project coverage is 98.75%. Comparing base (3dc1cd6) to head (cd8ad99).
Report is 99 commits behind head on master.

Files with missing lines Patch % Lines
context.go 82.69% 8 Missing and 1 partial ⚠️
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     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags "sonic avx" 98.74% <83.92%> (?)
-tags go_json 98.74% <83.92%> (?)
-tags nomsgpack 98.73% <83.92%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.23 98.75% <83.92%> (?)
go-1.24 98.75% <83.92%> (?)
macos-latest 98.75% <83.92%> (-0.47%) ⬇️
ubuntu-latest 98.75% <83.92%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rubensayshi
Copy link
Author

happy to increase the coverage if there's an actual chance this would get merged! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants