Avoid race condition crash on directory deletion #281
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
|
I believe (but am not certain) the test failure here is spurious due to general test flakiness. |
| # Directory may have been deleted between finding it in the directory | ||
| # list of its parent and trying to delete its contents. If this | ||
| # happens we treat it as empty, which is wrong but better than crashing | ||
| if e.errno == 2: |
There was a problem hiding this comment.
Compare with errno.ENOENT instead of hardcoding it
|
Thanks! Don't mind the occasional travis failure. It runs on a toaster so sometimes it's so slow it times out.
I understand your reasoning, but I don't think it's wrong to treat it as empty. The directory did exist the instance it was checked. It's the nature of snapshotting. In the next snapshot it will correct itself and report the dir as deleted. |
Given the directory structure /a/b/c.txt there is a race condition in DirectorySnapshot if it reads the list of entries in /a, then /a/b is deleted, and then it tries to read /a/b. This happens often in practice when changing between very different branches in git. The ideal behaviour would be to report /a/b as not existing in this case, but we cannot do this without either changing the order of the walk (from parent-first to parent-last) or significantly increasing memory usage and copies, so instead we report /a/b as existing but empty.
theospears
force-pushed
the
do-not-crash-on-dir-deletion
branch
from
b20604a to
2e83199
Compare
|
Updated to reflect comments. |
tamland
added a commit
that referenced
this pull request
Oct 25, 2014
Avoid race condition crash on directory deletion
CCP-Aporia
pushed a commit
to CCP-Aporia/watchdog
that referenced
this pull request
Aug 13, 2020
…ir-deletion Avoid race condition crash on directory deletion
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.
Given the directory structure /a/b/c.txt there is a race condition
in DirectorySnapshot if it reads the list of entries in /a, then
/a/b is deleted, and then it tries to read /a/b. This happens
often in practice when changing between very different branches in
git.
The correct behaviour would be to report /a/b as not existing in this
case, but we cannot do this without either changing the order of the
walk (from parent-first to parent-last) or significantly increasing
memory usage and copies, so instead we report /a/b as existing but
empty, which is not ideal, but is better than the current behavior
of silently crashing.