From 64257a396b644d63a9689fcbd84cca49ea31c386 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Sat, 16 Nov 2019 04:57:15 -0500 Subject: [PATCH 01/11] wrap existing rmtree to retry longer only for windows access errors. --- src/pip/_internal/utils/misc.py | 26 +++++++++++++++++++++++++- tests/unit/test_utils.py | 29 ++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index b84826350bc..51b00137c7e 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -131,10 +131,34 @@ def get_prog(): pass return 'pip' +def is_win_access_error(e): + """returns True if the OSError is a Windows access error which indicates + that another process is preventing the object from being deleted.""" + return e.errno == errno.EACCES and getattr(e, "winerror", None) == 5 + +def rmtree(dir, ignore_errors=False, n_tries=5): + """remove directory tree and on Windows will handle the case where + another process like a virus scanner is still holding onto a file + inside the target directory tree.""" + last_error = None + for i in range(n_tries): + try: + rmtree_internal(dir, ignore_errors) + return # succeeded, done. + except OSError as e: + if is_win_access_error(e): + logger.warning( + 'failed to remove %s - possibly held by another process - %s', + dir, e.strerror) + else: + raise + last_error = e + # re-raise last error if we've run out of attempts + raise last_error # Retry every half second for up to 3 seconds @retry(stop_max_delay=3000, wait_fixed=500) -def rmtree(dir, ignore_errors=False): +def rmtree_internal(dir, ignore_errors=False): # type: (str, bool) -> None shutil.rmtree(dir, ignore_errors=ignore_errors, onerror=rmtree_errorhandler) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index bbd48c6af98..5637dc92a8e 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -48,6 +48,7 @@ redact_netloc, remove_auth_from_url, rmtree, + rmtree_internal, rmtree_errorhandler, split_auth_from_netloc, split_auth_netloc_from_url, @@ -345,17 +346,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_internal.__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): @@ -374,6 +379,24 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch): with pytest.raises(OSError): rmtree('foo') +@pytest.mark.skipif("sys.platform != 'win32'") +def test_rmtree_retries_longer_for_winerror5(tmpdir, monkeypatch): + """ + Test pip._internal.utils.rmtree will retry longer for winerror5 + """ + winerror5 = OSError(13, 'Access is denied', 'foo', 5) + monkeypatch.setattr(shutil, 'rmtree', Failer(duration=4, exception=winerror5).call) + rmtree('foo') + +@pytest.mark.skipif("sys.platform != 'win32'") +def test_rmtree_retries_for_winerror5_can_be_limited(tmpdir, monkeypatch): + """ + Test pip._internal.utils.rmtree will retry longer for winerror5 + """ + winerror5 = OSError(13, 'Access is denied', 'foo', 5) + monkeypatch.setattr(shutil, 'rmtree', Failer(duration=8, exception=winerror5).call) + with pytest.raises(OSError): + rmtree('foo', n_tries=2) @pytest.mark.parametrize('path, fs_encoding, expected', [ (None, None, None), From 0c2262b190234ea76bfedaba6ee580b42b4fca08 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Sat, 16 Nov 2019 05:02:41 -0500 Subject: [PATCH 02/11] add news item --- news/7280.bugfix | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 news/7280.bugfix diff --git a/news/7280.bugfix b/news/7280.bugfix new file mode 100644 index 00000000000..4912aa5a840 --- /dev/null +++ b/news/7280.bugfix @@ -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. \ No newline at end of file From 9a577fba9594c0fe3f265e50cbfe23887a6a727c Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 18 Nov 2019 14:19:45 -0500 Subject: [PATCH 03/11] add newline at end of news. --- news/7280.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/7280.bugfix b/news/7280.bugfix index 4912aa5a840..22fb27a3e11 100644 --- a/news/7280.bugfix +++ b/news/7280.bugfix @@ -1,3 +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. \ No newline at end of file +holding onto files while pip tries to delete them. From 244181fc2871a5542c78639d8697c034e562da45 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 18 Nov 2019 14:22:18 -0500 Subject: [PATCH 04/11] rewrite based on suggestions from @chrahunt --- src/pip/_internal/utils/misc.py | 60 ++++++++++++++++++--------------- tests/unit/test_utils.py | 14 ++++---- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 51b00137c7e..067c815dc78 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -131,38 +131,44 @@ def get_prog(): pass return 'pip' -def is_win_access_error(e): - """returns True if the OSError is a Windows access error which indicates - that another process is preventing the object from being deleted.""" - return e.errno == errno.EACCES and getattr(e, "winerror", None) == 5 - -def rmtree(dir, ignore_errors=False, n_tries=5): - """remove directory tree and on Windows will handle the case where - another process like a virus scanner is still holding onto a file - inside the target directory tree.""" - last_error = None - for i in range(n_tries): - try: - rmtree_internal(dir, ignore_errors) - return # succeeded, done. - except OSError as e: - if is_win_access_error(e): - logger.warning( - 'failed to remove %s - possibly held by another process - %s', - dir, e.strerror) - else: - raise - last_error = e - # re-raise last error if we've run out of attempts - raise last_error - -# Retry every half second for up to 3 seconds +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_internal(dir, ignore_errors=False): +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": + def possibly_held_by_another_process(e): + """returns True if the error is a Windows access error which indicates + that another process is preventing the object from being deleted.""" + held_by_another = ( + isinstance(e, OSError) and + e.errno == errno.EACCES and + e.winerror == 5 + ) + if held_by_another: + logger.warning( + "%s (possibly held by another process). 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_another_process + )(rmtree_minimal_retry) def rmtree_errorhandler(func, path, exc_info): """On Windows, the files in .svn are read-only, so when rmtree() tries to diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 5637dc92a8e..91655dec779 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -48,7 +48,7 @@ redact_netloc, remove_auth_from_url, rmtree, - rmtree_internal, + rmtree_minimal_retry, rmtree_errorhandler, split_auth_from_netloc, split_auth_netloc_from_url, @@ -346,7 +346,7 @@ def test_rmtree_skips_nonexistent_directory(): Test wrapped rmtree doesn't raise an error by the given nonexistent directory. """ - rmtree_internal.__wrapped__('nonexistent-subdir') + rmtree_minimal_retry.__wrapped__('nonexistent-subdir') class Failer: @@ -379,13 +379,14 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch): with pytest.raises(OSError): rmtree('foo') +winerror5 = OSError(13, 'Access is denied', 'foo', 5) + @pytest.mark.skipif("sys.platform != 'win32'") def test_rmtree_retries_longer_for_winerror5(tmpdir, monkeypatch): """ Test pip._internal.utils.rmtree will retry longer for winerror5 """ - winerror5 = OSError(13, 'Access is denied', 'foo', 5) - monkeypatch.setattr(shutil, 'rmtree', Failer(duration=4, exception=winerror5).call) + monkeypatch.setattr(shutil, 'rmtree', Failer(duration=5, exception=winerror5).call) rmtree('foo') @pytest.mark.skipif("sys.platform != 'win32'") @@ -393,10 +394,9 @@ def test_rmtree_retries_for_winerror5_can_be_limited(tmpdir, monkeypatch): """ Test pip._internal.utils.rmtree will retry longer for winerror5 """ - winerror5 = OSError(13, 'Access is denied', 'foo', 5) - monkeypatch.setattr(shutil, 'rmtree', Failer(duration=8, exception=winerror5).call) + monkeypatch.setattr(shutil, 'rmtree', Failer(duration=17, exception=winerror5).call) with pytest.raises(OSError): - rmtree('foo', n_tries=2) + rmtree('foo') @pytest.mark.parametrize('path, fs_encoding, expected', [ (None, None, None), From 2d9866fb6f312fb0909fb327cc851a1dd2f53156 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 18 Nov 2019 15:54:46 -0500 Subject: [PATCH 05/11] make lint happy. --- src/pip/_internal/utils/misc.py | 11 ++++++++--- tests/unit/test_utils.py | 14 +++++++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 067c815dc78..7813f69eeac 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -131,11 +131,13 @@ def get_prog(): pass return 'pip' + 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 @@ -143,6 +145,7 @@ def rmtree_minimal_retry(dir, ignore_errors=False): 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. @@ -156,11 +159,12 @@ def possibly_held_by_another_process(e): isinstance(e, OSError) and e.errno == errno.EACCES and e.winerror == 5 - ) + ) if held_by_another: logger.warning( "%s (possibly held by another process). can't remove '%s'", - e.strerror, e.filename) + e.strerror, e.filename + ) return True else: return False @@ -168,7 +172,8 @@ def possibly_held_by_another_process(e): rmtree_impl = retry( stop_max_attempt_number=5, retry_on_exception=possibly_held_by_another_process - )(rmtree_minimal_retry) + )(rmtree_minimal_retry) + def rmtree_errorhandler(func, path, exc_info): """On Windows, the files in .svn are read-only, so when rmtree() tries to diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 91655dec779..d895cb5d990 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -48,8 +48,8 @@ redact_netloc, remove_auth_from_url, rmtree, - rmtree_minimal_retry, rmtree_errorhandler, + rmtree_minimal_retry, split_auth_from_netloc, split_auth_netloc_from_url, ) @@ -379,25 +379,33 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch): with pytest.raises(OSError): rmtree('foo') + winerror5 = OSError(13, 'Access is denied', 'foo', 5) + @pytest.mark.skipif("sys.platform != 'win32'") def test_rmtree_retries_longer_for_winerror5(tmpdir, monkeypatch): """ Test pip._internal.utils.rmtree will retry longer for winerror5 """ - monkeypatch.setattr(shutil, 'rmtree', Failer(duration=5, exception=winerror5).call) + monkeypatch.setattr( + shutil, 'rmtree', + Failer(duration=5, exception=winerror5).call) rmtree('foo') + @pytest.mark.skipif("sys.platform != 'win32'") def test_rmtree_retries_for_winerror5_can_be_limited(tmpdir, monkeypatch): """ Test pip._internal.utils.rmtree will retry longer for winerror5 """ - monkeypatch.setattr(shutil, 'rmtree', Failer(duration=17, exception=winerror5).call) + monkeypatch.setattr( + shutil, 'rmtree', + Failer(duration=17, exception=winerror5).call) with pytest.raises(OSError): rmtree('foo') + @pytest.mark.parametrize('path, fs_encoding, expected', [ (None, None, None), # Test passing a text (unicode) string. From 16b77d4652e37438d7b4ae83c95eae3aa42c2ea5 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Tue, 19 Nov 2019 02:16:07 -0500 Subject: [PATCH 06/11] differences between py27 & py3 on win32: update the code to handle the different errors for locked files on windows between python 2.7 and python 3. --- src/pip/_internal/utils/misc.py | 23 ++++++++++++++++------- tests/unit/test_utils.py | 25 ++++++++++++++++++------- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 7813f69eeac..1579d314359 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -152,17 +152,26 @@ def rmtree_minimal_retry(dir, ignore_errors=False): # 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 sys.version_info >= (3,): + def is_locked(e): + """on windows python 3, returns True if the OSError indicates + that another process has a handle on the file.""" + return e.errno == errno.EACCES and e.winerror == 5 + else: + def is_locked(e): + """on windows python 2.7, returns True if the OSError indicates + that another process has a handle on the file.""" + return e.errno == 41 and e.winerror == 145 + def possibly_held_by_another_process(e): """returns True if the error is a Windows access error which indicates that another process is preventing the object from being deleted.""" - held_by_another = ( - isinstance(e, OSError) and - e.errno == errno.EACCES and - e.winerror == 5 - ) - if held_by_another: + if not isinstance(e, OSError): + return False + if is_locked(e): logger.warning( - "%s (possibly held by another process). can't remove '%s'", + "%s (file may be locked by another process). " + "can't remove '%s'", e.strerror, e.filename ) return True diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index d895cb5d990..fb49842e69a 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -380,28 +380,39 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch): rmtree('foo') -winerror5 = OSError(13, 'Access is denied', 'foo', 5) +# windows access error for tests +if sys.platform == "win32": + if sys.version_info >= (3,): + # windows access error on python 3 + win32_access_error = OSError(13, 'Access is denied', 'foo', 5) + else: + # windows access error on python 2.7 + win32_access_error = WindowsError() + win32_access_error.errno = 41 + win32_access_error.strerror = 'The directory is not empty' + win32_access_error.filename = 'foo' + win32_access_error.winerror = 145 @pytest.mark.skipif("sys.platform != 'win32'") -def test_rmtree_retries_longer_for_winerror5(tmpdir, monkeypatch): +def test_rmtree_retry_extra_on_win32(tmpdir, monkeypatch): """ - Test pip._internal.utils.rmtree will retry longer for winerror5 + Test pip._internal.utils.rmtree will retry beyond 3 sec on windows. """ monkeypatch.setattr( shutil, 'rmtree', - Failer(duration=5, exception=winerror5).call) + Failer(duration=7, exception=win32_access_error).call) rmtree('foo') @pytest.mark.skipif("sys.platform != 'win32'") -def test_rmtree_retries_for_winerror5_can_be_limited(tmpdir, monkeypatch): +def test_rmtree_retry_limit_on_win32(tmpdir, monkeypatch): """ - Test pip._internal.utils.rmtree will retry longer for winerror5 + Test pip._internal.utils.rmtree will retry no more than 15 sec on windows. """ monkeypatch.setattr( shutil, 'rmtree', - Failer(duration=17, exception=winerror5).call) + Failer(duration=17, exception=win32_access_error).call) with pytest.raises(OSError): rmtree('foo') From 42665b2db7ea61b796f133d2e5091c595e1288cf Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Wed, 20 Nov 2019 04:28:53 -0500 Subject: [PATCH 07/11] cleaned up the tests. --- tests/unit/test_utils.py | 74 +++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 32 deletions(-) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index fb49842e69a..06bc6b7c9f1 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -380,42 +380,52 @@ def test_rmtree_retries_for_3sec(tmpdir, monkeypatch): rmtree('foo') -# windows access error for tests -if sys.platform == "win32": - if sys.version_info >= (3,): - # windows access error on python 3 - win32_access_error = OSError(13, 'Access is denied', 'foo', 5) - else: - # windows access error on python 2.7 - win32_access_error = WindowsError() - win32_access_error.errno = 41 - win32_access_error.strerror = 'The directory is not empty' - win32_access_error.filename = 'foo' - win32_access_error.winerror = 145 - - @pytest.mark.skipif("sys.platform != 'win32'") -def test_rmtree_retry_extra_on_win32(tmpdir, monkeypatch): - """ - Test pip._internal.utils.rmtree will retry beyond 3 sec on windows. - """ - monkeypatch.setattr( - shutil, 'rmtree', - Failer(duration=7, exception=win32_access_error).call) - rmtree('foo') +class TestVirusScanningIssuesOnWin32(object): + """tests to simulate rmtree problems caused by virus scanners preventing + files from being deleted.""" - -@pytest.mark.skipif("sys.platform != 'win32'") -def test_rmtree_retry_limit_on_win32(tmpdir, monkeypatch): - """ - Test pip._internal.utils.rmtree will retry no more than 15 sec on windows. - """ - monkeypatch.setattr( - shutil, 'rmtree', - Failer(duration=17, exception=win32_access_error).call) - with pytest.raises(OSError): + def test_rmtree_retry_beyond_minimum(self, monkeypatch): + """ + Test pip._internal.utils.rmtree will retry beyond 3 sec on windows. + """ + 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. + """ + 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=self.access_error()).call + + def access_error(self): + """simulated error raised when a privileged process is preventing + pip from deleting a file.""" + if sys.version_info >= (3,): + return self.access_error_on_py3() + else: + return self.access_error_on_py27() + + def access_error_on_py3(self): + e = OSError(13, 'Access is denied', 'foo', 5) + return e + + def access_error_on_py27(self): + e = WindowsError() + e.errno = 41 + e.strerror = 'The directory is not empty' + e.filename = 'foo' + e.winerror = 145 + return e + @pytest.mark.parametrize('path, fs_encoding, expected', [ (None, None, None), From 17816e826c6d9c278ec103c5b578d2e7f4dd286f Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Tue, 26 Nov 2019 01:42:43 -0500 Subject: [PATCH 08/11] use PY2; simplify is_locked inner function; as advised by @chrahunt. --- src/pip/_internal/utils/misc.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 1579d314359..75909e0c221 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -152,16 +152,15 @@ def rmtree_minimal_retry(dir, ignore_errors=False): # 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 sys.version_info >= (3,): - def is_locked(e): - """on windows python 3, returns True if the OSError indicates - that another process has a handle on the file.""" - return e.errno == errno.EACCES and e.winerror == 5 - else: - def is_locked(e): - """on windows python 2.7, returns True if the OSError indicates - that another process has a handle on the file.""" + def is_locked(e): + """ + on Windows, returns True if the OSError indicates that virus scanner + is preventing access to the file. + """ + if PY2: return e.errno == 41 and e.winerror == 145 + else: + return e.errno == errno.EACCES and e.winerror == 5 def possibly_held_by_another_process(e): """returns True if the error is a Windows access error which indicates From 61744fd6ae2d3f02ef43284d709878be0fa47ace Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 2 Dec 2019 02:30:47 -0500 Subject: [PATCH 09/11] better doc; use a constant. --- src/pip/_internal/utils/misc.py | 38 +++++++++++++++++++-------------- tests/unit/test_utils.py | 26 +++++----------------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 75909e0c221..6028cecc732 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -152,24 +152,30 @@ def rmtree_minimal_retry(dir, ignore_errors=False): # 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": - def is_locked(e): - """ - on Windows, returns True if the OSError indicates that virus scanner - is preventing access to the file. - """ - if PY2: - return e.errno == 41 and e.winerror == 145 - else: - return e.errno == errno.EACCES and e.winerror == 5 + 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 https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- + # 145 = ERROR_DIR_NOT_EMPTY: The directory is not empty. + VIRUS_SCAN_ERROR.winerror = 145 + else: + # from https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- + # 5 = ERROR_ACCESS_DENIED: Access is denied. + VIRUS_SCAN_ERROR = OSError(errno.EACCES, 'Access is denied', 'foo', 5) - def possibly_held_by_another_process(e): + def possibly_held_by_virus_scanner(e): """returns True if the error is a Windows access error which indicates - that another process is preventing the object from being deleted.""" - if not isinstance(e, OSError): - return False - if is_locked(e): + 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 (file may be locked by another process). " + "%s (virus scanner may be holding it). " "can't remove '%s'", e.strerror, e.filename ) @@ -179,7 +185,7 @@ def possibly_held_by_another_process(e): rmtree_impl = retry( stop_max_attempt_number=5, - retry_on_exception=possibly_held_by_another_process + retry_on_exception=possibly_held_by_virus_scanner )(rmtree_minimal_retry) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 06bc6b7c9f1..93ab6128289 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -55,6 +55,10 @@ ) 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" @@ -404,27 +408,7 @@ def test_rmtree_retry_limit(self, tmpdir, monkeypatch): def virus_scan_failure(self, seconds): """simulate rmtree failure caused by virus scanner for specified number of seconds.""" - return Failer(duration=seconds, exception=self.access_error()).call - - def access_error(self): - """simulated error raised when a privileged process is preventing - pip from deleting a file.""" - if sys.version_info >= (3,): - return self.access_error_on_py3() - else: - return self.access_error_on_py27() - - def access_error_on_py3(self): - e = OSError(13, 'Access is denied', 'foo', 5) - return e - - def access_error_on_py27(self): - e = WindowsError() - e.errno = 41 - e.strerror = 'The directory is not empty' - e.filename = 'foo' - e.winerror = 145 - return e + return Failer(duration=seconds, exception=VIRUS_SCAN_ERROR).call @pytest.mark.parametrize('path, fs_encoding, expected', [ From 1a8053d8fbe7e9b221ae200089e0fc2f491d63db Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 2 Dec 2019 02:44:08 -0500 Subject: [PATCH 10/11] explain the waits in the windows retry tests. --- tests/unit/test_utils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 93ab6128289..a01b545df80 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -393,6 +393,8 @@ 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') @@ -401,6 +403,8 @@ 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') From 078fa187c07e2efa103348b70c6d60b7afd136b4 Mon Sep 17 00:00:00 2001 From: Choy Rim Date: Mon, 2 Dec 2019 02:55:35 -0500 Subject: [PATCH 11/11] make lint happy --- src/pip/_internal/utils/misc.py | 4 ++-- tests/unit/test_utils.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 6028cecc732..13220ca6e8e 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -157,11 +157,11 @@ def rmtree_minimal_retry(dir, ignore_errors=False): VIRUS_SCAN_ERROR.errno = 41 VIRUS_SCAN_ERROR.strerror = 'The directory is not empty' VIRUS_SCAN_ERROR.filename = 'foo' - # from https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- + # from http://bit.ly/2r3ZEjt # 145 = ERROR_DIR_NOT_EMPTY: The directory is not empty. VIRUS_SCAN_ERROR.winerror = 145 else: - # from https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- + # from http://bit.ly/2r3ZEjt # 5 = ERROR_ACCESS_DENIED: Access is denied. VIRUS_SCAN_ERROR = OSError(errno.EACCES, 'Access is denied', 'foo', 5) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index a01b545df80..1c707c498d6 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -60,6 +60,7 @@ VIRUS_SCAN_ERROR ) + class Tests_EgglinkPath: "util.egg_link_path() tests"