backport-2.0: engine: actually unlock temp dirs on Windows #25439
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
File locks are mandatory on Windows; our cleanup code was previously assuming file system locks were advisory. Specifically, our cleanup code was locking the temporary directory, then attempting to remove it before releasing the lock. This worked on Unix platforms where locks are advisory-only, but failed on Windows because the process's own lock would prevent the cleanup! Presumably this ordering was meant to avoid a race condition where the directory was unlocked before it was cleaned up. It turns out this race condition doesn't matter (see the comment within), so just unlock the directory before removing it, which works on both Unix and Windows. Release note (bug fix): Restarting a CockroachDB server on Windows no longer fails due to file system locks in the store directory.
|
This change is |
bdarnell
approved these changes
May 13, 2018
|
bors r+ Merging this so it doesn't miss tomorrow's release cutoff. |
Build failed |
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
May 14, 2018
25439: backport-2.0: engine: actually unlock temp dirs on Windows r=bdarnell a=tschottdorf Backport 1/1 commits from #25267. /cc @cockroachdb/release --- File locks are mandatory on Windows; our cleanup code was previously assuming file system locks were advisory. Specifically, our cleanup code was locking the temporary directory, then attempting to remove it before releasing the lock. This worked on Unix platforms where locks are advisory-only, but failed on Windows because the process's own lock would prevent the cleanup! Presumably this ordering was meant to avoid a race condition where the directory was unlocked before it was cleaned up. It turns out this race condition doesn't matter (see the comment within), so just unlock the directory before removing it, which works on both Unix and Windows. Release note (bug fix): Restarting a CockroachDB server on Windows no longer fails due to file system locks in the store directory. Fix #24144. Fix #25272. Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Build succeeded |
|
First flake is
@bdarnell could you get in the habit of posting a quick note about the flake when retrying? I don't want us to get in the habit of retrying flakes away without taking action. |
|
Filed #25465 |
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.
Backport 1/1 commits from #25267.
/cc @cockroachdb/release
File locks are mandatory on Windows; our cleanup code was previously
assuming file system locks were advisory. Specifically, our cleanup code
was locking the temporary directory, then attempting to remove it before
releasing the lock. This worked on Unix platforms where locks are
advisory-only, but failed on Windows because the process's own lock
would prevent the cleanup! Presumably this ordering was meant to avoid a
race condition where the directory was unlocked before it was cleaned
up. It turns out this race condition doesn't matter (see the comment
within), so just unlock the directory before removing it, which works on
both Unix and Windows.
Release note (bug fix): Restarting a CockroachDB server on Windows no
longer fails due to file system locks in the store directory.
Fix #24144.
Fix #25272.