fix data race with batch reset #1150
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
this is another variation of the race found/fixed in blevesearch#1092 in that case the batch was empty, which meant we would skip the code that properly synchronized access. our fix only handled this exact case (no data operations), however there is another variation, if the batch contains only deletes (which are data ops) we still spawned the goroutine, although since there were no real updates, the again the synchronization code would be skipped, and thus the data race could happen. the fix is to check the number of updates (computed earlier on the caller's goroutine, so it's safe) instead of the length of the IndexOps (which includes updates and deletes) the key is that we should only spawn the goroutine that will range over the batch, in cases where we will synchronize on waiting for the analysis to complete (at least one update).
steveyen
approved these changes
Mar 3, 2019
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.
this is another variation of the race found/fixed in #1092
in that case the batch was empty, which meant we would skip
the code that properly synchronized access. our fix only
handled this exact case (no data operations), however
there is another variation, if the batch contains only deletes
(which are data ops) we still spawned the goroutine, although
since there were no real updates, the again the synchronization
code would be skipped, and thus the data race could happen.
the fix is to check the number of updates (computed earlier on
the caller's goroutine, so it's safe) instead of the length
of the IndexOps (which includes updates and deletes)
the key is that we should only spawn the goroutine that will
range over the batch, in cases where we will synchronize on
waiting for the analysis to complete (at least one update).