-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rmtree failures on Windows (#1031) #1032
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import shutil | ||
import stat | ||
import tempfile | ||
import time | ||
|
||
from contextlib import contextmanager | ||
from typing import List | ||
|
@@ -29,17 +30,32 @@ def normalize_version(version): # type: (str) -> str | |
|
||
@contextmanager | ||
def temporary_directory(*args, **kwargs): | ||
try: | ||
from tempfile import TemporaryDirectory | ||
name = tempfile.mkdtemp(*args, **kwargs) | ||
|
||
with TemporaryDirectory(*args, **kwargs) as name: | ||
yield name | ||
except ImportError: | ||
name = tempfile.mkdtemp(*args, **kwargs) | ||
yield name | ||
|
||
yield name | ||
robust_rmtree(name) | ||
|
||
shutil.rmtree(name) | ||
|
||
def robust_rmtree(path): | ||
"""Robustly tries to delete paths. | ||
|
||
Retries several times if an OSError occurs. | ||
If the final attempt fails, the Exception is propagated | ||
to the caller. | ||
""" | ||
timeout = 0.001 | ||
while timeout < 2: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why 2 seconds instead 1 like the python version? Without a strong justification otherwise, I think we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was running 2 seconds simply becuase I felt that it wasn't an unreasonable wait that little extra longer to increase robustness when this is simply gating a crash situation (i.e. the program will error at the end of the timeout if it fails). CPython is using it in their tests, so obviously they are much more concious of the tradeoff between robustness and speed. That being said, 1s is almost certainly enough so I'm fine switching it to that. And we can increase it in the future if people still face the issue. |
||
try: | ||
shutil.rmtree(path) | ||
return # Only hits this on success | ||
except OSError: | ||
# Increase the timeout and try again | ||
time.sleep(timeout) | ||
timeout *= 2 | ||
|
||
# Final attempt, pass any Exceptions up to caller. | ||
shutil.rmtree(path) | ||
|
||
|
||
def parse_requires(requires): # type: (str) -> List[str] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to match the style of the codebase, please put this on a newline