Fix #1870: Race condition with some virus scanners #1873
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
Some virus scanners take out handles on files we are going to rename, causing access errors and preventing the rename. Typically this is short lived. Retry rather than erroring. No feedback given at the moment, and it will be a much larger patch to do so - as this is a regression fix I'd like to get it in and released and follow up with something thread all the UI channels down to this layer later (e.g. an implicit context mechanism of some sort perhaps?)
kinnison
suggested changes
May 25, 2019
Contributor
kinnison
left a comment
kinnison
left a comment
There was a problem hiding this comment.
Looks sound, thank you for doing the work with the retry crate too.
One point to address
Contributor
Author
|
This will happen to wsl v1 users on windows too as they share a file system.
…On Sun, 26 May 2019, 07:24 Daniel Silverstone, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Looks sound, thank you for doing the work with the retry crate too.
One point to address
------------------------------
In src/utils/utils.rs
<#1873 (comment)>:
> @@ -651,7 +654,21 @@ pub fn toolchain_sort<T: AsRef<str>>(v: &mut Vec<T>) {
}
fn rename(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
- fs::rename(src, dest).chain_err(|| ErrorKind::RenamingFile {
+ // #1870
+ // 21 fib steps from 1 sums to ~28 seconds, hopefully more than enough
+ // for our previous poor performance that avoided the race condition with
+ // McAfee and Norton.
+ retry(
+ Fibonacci::from_millis(1).map(jitter).take(21),
+ || match fs::rename(src, dest) {
+ Ok(v) => OperationResult::Ok(v),
+ Err(e) => match e.kind() {
+ io::ErrorKind::PermissionDenied => OperationResult::Retry(e),
I suppose, on a Linux system (or mac) where the EPERM was actually more
to do with someone chown()ing their stuff oddly, this will end up
consuming 28s before failing -- should we fail faster by making this
somehow conditional on Windows?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1873>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADZ7XW3FT4EH7OVHVW7EJTPXGG77ANCNFSM4HPTWVRA>
.
|
Contributor
Accepted. |
kinnison
approved these changes
May 25, 2019
Contributor
kinnison
left a comment
kinnison
left a comment
There was a problem hiding this comment.
If we get a green CI, this LGTM.
Contributor
|
I think the appveyor failure is spurious. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Some virus scanners take out handles on files we are going to
rename, causing access errors and preventing the rename. Typically
this is short lived. Retry rather than erroring.
No feedback given at the moment, and depends on an unreleased
retry, so WIP...