Skip to content
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

Add before you push to the queue to prevent race condition on size #2044

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

joe-elliott
Copy link
Member

Which problem is this PR solving?

Fixes #2042

Short description of the changes

Adding to size before we put an item in the channel prevents q.size.Sub(1) hitting before the addition. This prevents the uint from underflowing to max value.

This change fixed the issue described in #2042 in our environment.

cc @gouthamve

@joe-elliott joe-elliott requested a review from a team as a code owner January 27, 2020 16:45
@joe-elliott
Copy link
Member Author

joe-elliott commented Jan 27, 2020

Submodule updated. Fixing.
Fixed.

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #2044 into master will decrease coverage by <.01%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2044      +/-   ##
==========================================
- Coverage    97.4%   97.39%   -0.01%     
==========================================
  Files         207      207              
  Lines       10315    10286      -29     
==========================================
- Hits        10047    10018      -29     
  Misses        224      224              
  Partials       44       44
Impacted Files Coverage Δ
cmd/agent/app/reporter/grpc/flags.go 100% <ø> (ø) ⬆️
cmd/collector/app/builder/builder_flags.go 100% <ø> (ø) ⬆️
cmd/collector/app/span_processor.go 98.43% <0%> (-1.57%) ⬇️
plugin/storage/es/options.go 100% <100%> (ø) ⬆️
pkg/config/tlscfg/options.go 100% <100%> (ø) ⬆️
plugin/storage/cassandra/options.go 100% <100%> (ø) ⬆️
pkg/config/tlscfg/flags.go 100% <100%> (ø) ⬆️
cmd/query/app/server.go 94.44% <0%> (+2.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aaa413...332fd88. Read the comment docs.

@joe-elliott joe-elliott changed the title Add before you push to the queue to prevent data race on size Add before you push to the queue to prevent race condition on size Jan 27, 2020
@@ -105,12 +105,13 @@ func (q *BoundedQueue) Produce(item interface{}) bool {
return false
}

q.size.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually looks like a real bug, we are subtracting in both code branches, but adding in only one. Good catch!

@jpkrohling jpkrohling merged commit 0e974ce into jaegertracing:master Jan 28, 2020
@jpkrohling
Copy link
Contributor

Thanks for the PR!

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.

Dropping small amount of spans on master
2 participants