Fix race listing readable files #198
Merged
Conversation
This file contains hidden or 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 test shows what happens when listing the readable files from a Bitcask directory. The list should not include the active write or merge files. But if those change after the code fetches the name of the current one, the listing will contain the new active write and/or merge files, skipping the *previous* write or merge file. That would be bad. The test uses meck to return the filename, then in one go write a new one to simulate the race.
A race existed as demonstrated by the newly added list_data_files_race_test test. When listing readable files from a Bitcask directory for merging and such, the current write or merge output files should never be listed along with the other files. This could lead to merges trying to read active files while they are being written. Now, the code fetches the current merge and write files, lists the directory contents, filtering those out, and checks again if the merge and write file have changed. If they have, we try again, only succeeding if no changes have happened in between.
|
+1 694480f I disagree that the old code was crazy: it was detecting if a race happened in a different way. As you found, it wasn't used consistently, which was bad. Review + 10 hours QuickCheck+PULSE look fine. |
borshop
added a commit
that referenced
this pull request
Sep 17, 2014
Fix race listing readable files Reviewed-by: slfritchie
|
@borshop merge |
|
Let's discuss the insanity of that code after a few beers one of these days. Thanks! |
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.
A race existed as demonstrated by the newly added
list_data_files_race_test test. When listing readable files from a
Bitcask directory for merging and such, the current write or merge
output files should never be listed along with the other files. This
could lead to merges trying to read active files while they are being
written.
Now, the code fetches the current merge and write files, lists the
directory contents, filtering those out, and checks again if the merge
and write file have changed. If they have, we try again, only succeeding
if no changes have happened in between.
This PR also removes some code that seems to indicate somebody saw
the race at some point. Alas, it was fixed in a crazy, crazy way.
This addresses issue #194