return queue is full error if sized_channel is full #11063
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11063 +/- ##
==========================================
- Coverage 92.23% 92.21% -0.02%
==========================================
Files 405 414 +9
Lines 19089 19728 +639
==========================================
+ Hits 17606 18193 +587
- Misses 1123 1165 +42
- Partials 360 370 +10 ☔ View full report in Codecov by Sentry. |
splunkericl
force-pushed
the
persistent_queue_deadlock
branch
from
498c230 to
678ef6a
Compare
splunkericl
force-pushed
the
persistent_queue_deadlock
branch
from
678ef6a to
09ce9d3
Compare
|
cc @sfc-gh-sili as well as snowflake partner is helping out |
dmitryax
reviewed
Sep 6, 2024
dmitryax
reviewed
Sep 6, 2024
splunkericl
added a commit
to splunkericl/opentelemetry-collector
that referenced
this pull request
Sep 6, 2024
…1063) #### Description This change fixes a potential deadlock bug for persistent queue. There is a race condition in persistent queue that caused `used` in `sizedChannel` to become out of sync with `ch` len. This causes `Offer` to be deadlocked in specific race condition. For example: 1. Multiple consumers are calling Consume 2. Multiple producers are calling Offer to insert into the queue a. All elements are taken from consumers. ch is empty 3. One consumer completes consume, calls onProcessingFinished a. Inside sizedChannel, syncSize is invoked, used is reset to 0 when other consumers are still waiting for lock to consume 4. More Offer is called inserting elements -> used and ch len should equal 5. As step 3a consumers completes, used is decreased -> used is lower than ch len a. More Offer is called inserting since used is below capacity. however, ch is full. b. goroutine calling offer is holding the mutex but can’t release it as ch is full. c. no consumer can acquire mutex to complete previous onProcessingFinished This change returns an error if channel is full instead of waiting for it to unblock. #### Link to tracking issue Fixes # open-telemetry#11015 #### Testing - Added concurrent test in persistent queue that can reproduce the problem(note: need to re-run it 100 times as the race condition is not consistent). - Added unit test for sizedChannel #### Documentation Added comment in the block explaining it --------- Co-authored-by: Dmitrii Anoshin <[email protected]>
splunkericl
added a commit
to splunkericl/opentelemetry-collector
that referenced
this pull request
Oct 22, 2024
…1063) #### Description This change fixes a potential deadlock bug for persistent queue. There is a race condition in persistent queue that caused `used` in `sizedChannel` to become out of sync with `ch` len. This causes `Offer` to be deadlocked in specific race condition. For example: 1. Multiple consumers are calling Consume 2. Multiple producers are calling Offer to insert into the queue a. All elements are taken from consumers. ch is empty 3. One consumer completes consume, calls onProcessingFinished a. Inside sizedChannel, syncSize is invoked, used is reset to 0 when other consumers are still waiting for lock to consume 4. More Offer is called inserting elements -> used and ch len should equal 5. As step 3a consumers completes, used is decreased -> used is lower than ch len a. More Offer is called inserting since used is below capacity. however, ch is full. b. goroutine calling offer is holding the mutex but can’t release it as ch is full. c. no consumer can acquire mutex to complete previous onProcessingFinished This change returns an error if channel is full instead of waiting for it to unblock. #### Link to tracking issue Fixes # open-telemetry#11015 #### Testing - Added concurrent test in persistent queue that can reproduce the problem(note: need to re-run it 100 times as the race condition is not consistent). - Added unit test for sizedChannel #### Documentation Added comment in the block explaining it --------- Co-authored-by: Dmitrii Anoshin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change fixes a potential deadlock bug for persistent queue.
There is a race condition in persistent queue that caused
usedinsizedChannelto become out of sync withchlen. This causesOfferto be deadlocked in specific race condition. For example:a. All elements are taken from consumers. ch is empty
a. Inside sizedChannel, syncSize is invoked, used is reset to 0 when other consumers are still waiting for lock to consume
a. More Offer is called inserting since used is below capacity. however, ch is full.
b. goroutine calling offer is holding the mutex but can’t release it as ch is full.
c. no consumer can acquire mutex to complete previous onProcessingFinished
This change returns an error if channel is full instead of waiting for it to unblock.
Link to tracking issue
Fixes # #11015
Testing
Documentation
Added comment in the block explaining it