-
Notifications
You must be signed in to change notification settings - Fork 573
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
Use an atomic.Int64 as bodyWrapper read bytes counter #5080
Use an atomic.Int64 as bodyWrapper read bytes counter #5080
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5080 +/- ##
=====================================
Coverage 79.6% 79.6%
=====================================
Files 152 152
Lines 10511 10511
=====================================
Hits 8371 8371
Misses 1944 1944
Partials 196 196
|
…#5080) * use an atomic.Int64 as bodyWrapper read bytes counter * add changelog entry --------- Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
I think this fix may lead to incorrect metric reported, see my comment here #4895 (comment). This can be checked by having a reader for the body that is slow+a big body and a server that starts responding without consuming all of the incoming request's body. |
I think this is still as improvement worth including. Slightly incorrect data is better then a panic. Please do submit a change to address the data inconsistency you mention. |
I agree this is an improvement vs a data race. I'm not planning to work on this, but I've opened an issue so that this is not lost and someone can pick it up #5087. |
Closes #4895.
This switches the bodyWrapper struct to use an
atomic.Int64
instead of a plainint64
integer to count read bytes.This should solve the race condition mentioned in #4895, though unfortunately, this isn't being tested as I haven't been able to reproduce the issue within the codebase.