Skip to content

7280 alternative fix: wrap existing rmtree to retry longer only for windows access errors. #7363

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

Closed
wants to merge 11 commits into from
3 changes: 3 additions & 0 deletions news/7280.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Wrap rmtree again so that it does special handling only for removing files
on windows where often virus scanners can interfere with file deletion by
holding onto files while pip tries to delete them.
53 changes: 51 additions & 2 deletions src/pip/_internal/utils/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,63 @@ def get_prog():
return 'pip'


# Retry every half second for up to 3 seconds
@retry(stop_max_delay=3000, wait_fixed=500)
def rmtree(dir, ignore_errors=False):
# type: (str, bool) -> None
"""remove directory tree, with appropriate retry logic per OS."""
rmtree_impl(dir, ignore_errors)


@retry(stop_max_delay=3000, wait_fixed=500)
def rmtree_minimal_retry(dir, ignore_errors=False):
# type: (str, bool) -> None
"""remove directory tree, retry up to 3 seconds."""
shutil.rmtree(dir, ignore_errors=ignore_errors,
onerror=rmtree_errorhandler)


# default rmtree implementation
rmtree_impl = rmtree_minimal_retry
# on win32, wrap rmtree with more retries.
# this helps remediate the case when another when a virus scanner (or any
# other process) is still holding a handle to the file being deleted.
if sys.platform == "win32":
if PY2:
VIRUS_SCAN_ERROR = WindowsError()
VIRUS_SCAN_ERROR.errno = 41
VIRUS_SCAN_ERROR.strerror = 'The directory is not empty'
VIRUS_SCAN_ERROR.filename = 'foo'
# from http://bit.ly/2r3ZEjt
# 145 = ERROR_DIR_NOT_EMPTY: The directory is not empty.
VIRUS_SCAN_ERROR.winerror = 145
else:
# from http://bit.ly/2r3ZEjt
# 5 = ERROR_ACCESS_DENIED: Access is denied.
VIRUS_SCAN_ERROR = OSError(errno.EACCES, 'Access is denied', 'foo', 5)

def possibly_held_by_virus_scanner(e):
"""returns True if the error is a Windows access error which indicates
that a virus scanner is preventing the object from being deleted."""
is_denied_access = (
isinstance(e, OSError) and
e.errno == VIRUS_SCAN_ERROR.errno and
e.winerror == VIRUS_SCAN_ERROR.winerror
)
if is_denied_access:
logger.warning(
"%s (virus scanner may be holding it). "
"can't remove '%s'",
e.strerror, e.filename
)
return True
else:
return False

rmtree_impl = retry(
stop_max_attempt_number=5,
retry_on_exception=possibly_held_by_virus_scanner
)(rmtree_minimal_retry)


def rmtree_errorhandler(func, path, exc_info):
"""On Windows, the files in .svn are read-only, so when rmtree() tries to
remove them, an exception is thrown. We catch that here, remove the
Expand Down
47 changes: 44 additions & 3 deletions tests/unit/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,17 @@
remove_auth_from_url,
rmtree,
rmtree_errorhandler,
rmtree_minimal_retry,
split_auth_from_netloc,
split_auth_netloc_from_url,
)
from pip._internal.utils.setuptools_build import make_setuptools_shim_args

if sys.platform == "win32":
from pip._internal.utils.misc import (
VIRUS_SCAN_ERROR
)


class Tests_EgglinkPath:
"util.egg_link_path() tests"
Expand Down Expand Up @@ -345,17 +351,21 @@ def test_rmtree_skips_nonexistent_directory():
Test wrapped rmtree doesn't raise an error
by the given nonexistent directory.
"""
rmtree.__wrapped__('nonexistent-subdir')
rmtree_minimal_retry.__wrapped__('nonexistent-subdir')


class Failer:
def __init__(self, duration=1):
def __init__(self, duration=1, exception=None):
self.succeed_after = time.time() + duration
self.exception = exception

def call(self, *args, **kw):
"""Fail with OSError self.max_fails times"""
if time.time() < self.succeed_after:
raise OSError("Failed")
if self.exception is None:
raise OSError("Failed")
else:
raise self.exception


def test_rmtree_retries(tmpdir, monkeypatch):
Expand All @@ -375,6 +385,37 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch):
rmtree('foo')


@pytest.mark.skipif("sys.platform != 'win32'")
class TestVirusScanningIssuesOnWin32(object):
"""tests to simulate rmtree problems caused by virus scanners preventing
files from being deleted."""

def test_rmtree_retry_beyond_minimum(self, monkeypatch):
"""
Test pip._internal.utils.rmtree will retry beyond 3 sec on windows.
"""
# the 5 here is 2 sec more than the 3 sec done by the standard
# retries tested in test_rmtree_retries_for_3sec
monkeypatch.setattr(shutil, 'rmtree', self.virus_scan_failure(5))
rmtree('foo')

def test_rmtree_retry_limit(self, tmpdir, monkeypatch):
"""
Test pip._internal.utils.rmtree will retry no more than 15 sec on
windows.
"""
# the 17 here is 2 sec more than the total time waited by
# retrying 5 times the standard 3 sec wait.
monkeypatch.setattr(shutil, 'rmtree', self.virus_scan_failure(17))
with pytest.raises(OSError):
rmtree('foo')

def virus_scan_failure(self, seconds):
"""simulate rmtree failure caused by virus scanner for specified
number of seconds."""
return Failer(duration=seconds, exception=VIRUS_SCAN_ERROR).call


@pytest.mark.parametrize('path, fs_encoding, expected', [
(None, None, None),
# Test passing a text (unicode) string.
Expand Down