From ae887ac51c38049c899161e1e5fd173fe40d40c9 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Mon, 30 Dec 2019 18:21:40 -0500 Subject: [PATCH 1/9] Addresses #7280 - More targeted fix to problem - Try to delete the temporary directory, as most of the time this works - If it fails on windows due to an Access error, it clearly is not an Access error because we just created the directory. Warn the user and continue. --- src/pip/_internal/utils/temp_dir.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 0be0ff785cd..29fd8097390 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -4,6 +4,7 @@ import itertools import logging import os.path +import sys import tempfile from contextlib import contextmanager @@ -124,7 +125,21 @@ def cleanup(self): """ self._deleted = True if os.path.exists(self._path): - rmtree(self._path) + try: + rmtree(self._path) + except OSError as e: + skip_error = ( + sys.platform == 'win32' and + e.errno == errno.EACCES and + getattr(e, 'winerror', 0) in {5, 32} + ) + if skip_error: + logger.warning( + "%s (virus scanner may be holding it)." + "cannot remove '%s'", + e.strerror, e.filename) + else: + raise class AdjacentTempDirectory(TempDirectory): From b90cc2c84c1659a5820768a1ddd934ad1c510246 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Mon, 30 Dec 2019 18:25:23 -0500 Subject: [PATCH 2/9] Address #7280 - Add issue description file - Add issue description file --- news/7280.bugfix | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 news/7280.bugfix diff --git a/news/7280.bugfix b/news/7280.bugfix new file mode 100644 index 00000000000..6f536365583 --- /dev/null +++ b/news/7280.bugfix @@ -0,0 +1,2 @@ +When pip is installing or removing a package and is unable to remove a temporary directory on Windows, this is likely due to a Virus Scan operation. +Warn the user and continue rather than preventing the operation. From 942ab14f00c59d80d5536dae099afca07ac4499e Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Tue, 31 Dec 2019 17:30:09 -0500 Subject: [PATCH 3/9] Hardening test cases for #7280 - Provide a class that manages a sub-process which can open a file and monkey with pip's abiliity to remove a file on windows. - This class can function as both a context manager and/or a pytest fixture. --- tests/lib/filesystem.py | 91 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index dc14b323e33..a33a703f951 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -1,9 +1,11 @@ """Helpers for filesystem-dependent tests. """ import os +import multiprocessing import socket import subprocess import sys +import traceback from functools import partial from itertools import chain @@ -34,6 +36,95 @@ def make_unreadable_file(path): subprocess.check_call(args) +if sys.platform == 'win32': + def lock_action(f): + pass +else: + def lock_action(f): + pass + + +def external_file_opener(conn): + """ + This external process is run with multiprocessing. + It waits for a path from the parent, opens it, and then wait for another message before closing it. + + :param conn: bi-directional pipe + :return: nothing + """ + f = None + try: + # Wait for parent to send path + msg = conn.recv() + if msg is True: + # Do nothing - we have been told to exit without a path or action + pass + else: + path, action = msg + # Open the file + try: + f = open(path, 'r') + if action == 'lock': + lock_action(f) + elif action == 'noread': + make_unreadable_file(path) + except OSError as e: + traceback.print_exc(None, sys.stderr) + + # Indicate the file is opened + conn.send(True) + # Now path is open and we wait for signal to exit + conn.recv() + finally: + if f: + f.close() + conn.close() + + +class FileOpener(object): + """ + Test class acts as a context manager which can open a file from another process, and hold it open + to assure that this does not interfere with pip's operations. + + If a path is passed to the FileOpener, it immediately sends a message to the other process to + open that path. An action of "lock" or "noread" can also be sent to the subprocess, resulting + in various additional monkey wrenches now and in the future. + + Opening the path and taking the action can be deferred however, so that the FileOpener may + function as a pytest fixture if so desired. + """ + def __init__(self, + path=None, # type: Optional(str) + action=None, # type: Optional(str) + ): + self.path = None + self.conn, child_conn = multiprocessing.Pipe() + self.child = multiprocessing.Process(target=external_file_opener, daemon=True, args=(child_conn,)) + self.child.start() + if path: + self.send(path, action) + + def send(self, path, action=None): + if self.path is not None: + raise AttributeError('path may only be set once') + self.path = str(path) + self.conn.send( (str(path), action) ) + return self.conn.recv() + + def cleanup(self): + # send a message to the child to exit; it will exit whether the path was sent or not + if self.child: + self.conn.send(True) + self.child.join() + self.child = None + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + self.cleanup() + + def get_filelist(base): def join(dirpath, dirnames, filenames): relative_dirpath = os.path.relpath(dirpath, base) From cce077fa93b2b3e1b9c35558279ff9d9e2d69884 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 12:14:06 -0500 Subject: [PATCH 4/9] Fix issue #7280 - capture error and warn, but retry as normal - TempDirectory() tries to delete the directory as normal - if cleanup fails on Windows due to EACCES, warn about virus scanner - This is a more specific error than previous error, but does not change the behavior when a user is attempting to pip uninstall in a system directory - This changes the messaging, but risks leaving the directory behind. - Leaving the directory behind, however, is what is already happening - The fix for #7479 gives the Virus scanner more time to finish its work, but there is still a possibility for a race condition that leaves the impression that there is an error in pip itself. --- src/pip/_internal/utils/temp_dir.py | 7 +--- tests/lib/filesystem.py | 1 + tests/unit/test_utils_filesystem.py | 56 ++++++++++++++++++++++++++++- tests/unit/test_utils_temp_dir.py | 35 ++++++++++++++++++ tools/requirements/tests.txt | 2 ++ 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 29fd8097390..1f80cb48f56 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -128,12 +128,7 @@ def cleanup(self): try: rmtree(self._path) except OSError as e: - skip_error = ( - sys.platform == 'win32' and - e.errno == errno.EACCES and - getattr(e, 'winerror', 0) in {5, 32} - ) - if skip_error: + if sys.platform == 'win32' and e.errno == errno.EACCES: logger.warning( "%s (virus scanner may be holding it)." "cannot remove '%s'", diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index a33a703f951..983e95e0ae2 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -64,6 +64,7 @@ def external_file_opener(conn): # Open the file try: f = open(path, 'r') + # NOTE: action is for future use and may be unused if action == 'lock': lock_action(f) elif action == 'noread': diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index 3ef814dce4b..fc336e23add 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -1,13 +1,19 @@ import os import shutil +import psutil import pytest from pip._internal.utils.filesystem import copy2_fixed, is_socket -from tests.lib.filesystem import make_socket_file, make_unreadable_file +from tests.lib.filesystem import make_socket_file, make_unreadable_file, FileOpener from tests.lib.path import Path +@pytest.fixture() +def process(): + return psutil.Process() + + def make_file(path): Path(path).touch() @@ -27,6 +33,7 @@ def make_dir(path): skip_on_windows = pytest.mark.skipif("sys.platform == 'win32'") +skip_unless_windows = pytest.mark.skipif("sys.platform != 'win32'") @skip_on_windows @@ -59,3 +66,50 @@ def test_copy2_fixed_raises_appropriate_errors(create, error_type, tmpdir): copy2_fixed(src, dest) assert not dest.exists() + + +def test_file_opener_no_file(process): + # The FileOpener cleans up the subprocess even if the parent never sends the path + with FileOpener(): + pass + assert len(process.children()) == 0 + + +def test_file_opener_not_found(tmpdir, process): + # The FileOpener cleans up the subprocess even if the file cannot be opened + path = tmpdir.joinpath('foo.txt') + with FileOpener(path): + pass + assert len(process.children()) == 0 + + +def test_file_opener_normal(tmpdir, process): + # The FileOpener cleans up the subprocess when the file exists; also tests that path may be deferred + path = tmpdir.joinpath('foo.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener() as opener: + opener.send(path) + assert len(process.children()) == 0 + + +@skip_unless_windows +def test_file_opener_produces_unlink_error(tmpdir, process): + path = tmpdir.joinpath('foo.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener(path): + with pytest.raises(OSError): + os.unlink(path) + + +@skip_unless_windows +def test_file_opener_produces_rmtree_error(tmpdir, process): + subdir = tmpdir.joinpath('foo') + os.mkdir(subdir) + path = subdir.joinpath('bar.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener(path): + with pytest.raises(OSError): + shutil.rmtree(subdir) diff --git a/tests/unit/test_utils_temp_dir.py b/tests/unit/test_utils_temp_dir.py index 9b45a75b9e2..5967ff8606b 100644 --- a/tests/unit/test_utils_temp_dir.py +++ b/tests/unit/test_utils_temp_dir.py @@ -1,7 +1,10 @@ import itertools +import logging import os +import shutil import stat import tempfile +import time import pytest @@ -13,6 +16,8 @@ global_tempdir_manager, ) +from tests.lib.filesystem import FileOpener + # No need to test symlinked directories on Windows @pytest.mark.skipif("sys.platform == 'win32'") @@ -207,3 +212,33 @@ def test_tempdirectory_asserts_global_tempdir(monkeypatch): monkeypatch.setattr(temp_dir, "_tempdir_manager", None) with pytest.raises(AssertionError): TempDirectory(globally_managed=True) + + +@pytest.mark.skipif("sys.platform != 'win32'") +def test_temp_dir_warns_if_cannot_clean(caplog): + temp_dir = TempDirectory() + temp_dir_path = temp_dir.path + + stime = time.time() + + # Capture only at WARNING level and up + with caplog.at_level(logging.WARNING, 'pip._internal.utils.temp_dir'): + # open a file within the temporary directory in a sub-process, so it cannot be cleaned up + with FileOpener() as opener: + subpath = os.path.join(temp_dir_path, 'foo.txt') + with open(subpath, 'w') as f: + f.write('Cannot be deleted') + opener.send(subpath) + # with the file open, attempt to remove the log directory + temp_dir.cleanup() + + # assert that a WARNING was logged and that it contains a note about a potential virus scanner + assert 'WARNING' in caplog.text + assert 'virus scanner' in caplog.text + + # Assure that the cleanup was properly retried; this fix did not change retries + duration = time.time() - stime + assert duration >= 2.0 + + # Clean-up for failed TempDirectory cleanup + shutil.rmtree(temp_dir_path, ignore_errors=True) diff --git a/tools/requirements/tests.txt b/tools/requirements/tests.txt index 747ea321fb6..4e3fc9bd413 100644 --- a/tools/requirements/tests.txt +++ b/tools/requirements/tests.txt @@ -2,6 +2,8 @@ cryptography==2.8 freezegun mock pretend +# Below is to count sub-processes and assure child was cleaned up properly +psutil pytest==3.8.2 pytest-cov # Prevent installing 7.0 which has install_requires "pytest >= 3.10". From b6c09914ae6e413f70911931f28b359f410dc551 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 12:21:36 -0500 Subject: [PATCH 5/9] Update news entry for #7280 to be more complete. --- news/7280.bugfix | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/news/7280.bugfix b/news/7280.bugfix index 6f536365583..17ac0cd2f55 100644 --- a/news/7280.bugfix +++ b/news/7280.bugfix @@ -1,2 +1,4 @@ -When pip is installing or removing a package and is unable to remove a temporary directory on Windows, this is likely due to a Virus Scan operation. -Warn the user and continue rather than preventing the operation. +When pip is installing or removing a package and is unable to remove a temporary directory on Windows, +this is likely due to a Virus Scan operation. This fix warns the user and continues rather than +preventing the operation. pip still retries the removal, so this happens quite rarely in most Windows +environments. From eb7375255f735a1f5bc20b561172c11ba44cc08f Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 13:08:35 -0500 Subject: [PATCH 6/9] Issue #7280 - Clean-up python2.7, isort, and flake8 errors --- tests/lib/filesystem.py | 38 ++++++++++++++++------------- tests/unit/test_utils_filesystem.py | 21 ++++++++++------ tests/unit/test_utils_temp_dir.py | 7 +++--- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index 983e95e0ae2..72b532121ad 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -1,7 +1,7 @@ """Helpers for filesystem-dependent tests. """ -import os import multiprocessing +import os import socket import subprocess import sys @@ -47,7 +47,8 @@ def lock_action(f): def external_file_opener(conn): """ This external process is run with multiprocessing. - It waits for a path from the parent, opens it, and then wait for another message before closing it. + It waits for a path from the parent, opens it, and then wait for another + message before closing it. :param conn: bi-directional pipe :return: nothing @@ -69,7 +70,7 @@ def external_file_opener(conn): lock_action(f) elif action == 'noread': make_unreadable_file(path) - except OSError as e: + except OSError: traceback.print_exc(None, sys.stderr) # Indicate the file is opened @@ -84,23 +85,26 @@ def external_file_opener(conn): class FileOpener(object): """ - Test class acts as a context manager which can open a file from another process, and hold it open - to assure that this does not interfere with pip's operations. + Test class acts as a context manager which can open a file from a + subprocess, and hold it open to assure that this does not interfere with + pip's operations. - If a path is passed to the FileOpener, it immediately sends a message to the other process to - open that path. An action of "lock" or "noread" can also be sent to the subprocess, resulting - in various additional monkey wrenches now and in the future. + If a path is passed to the FileOpener, it immediately sends a message to + the other process to open that path. An action of "lock" or "noread" can + also be sent to the subprocess, resulting in various additional monkey + wrenches now and in the future. - Opening the path and taking the action can be deferred however, so that the FileOpener may - function as a pytest fixture if so desired. + Opening the path and taking the action can be deferred however, so that + the FileOpener may function as a pytest fixture if so desired. """ - def __init__(self, - path=None, # type: Optional(str) - action=None, # type: Optional(str) - ): + def __init__(self, path=None, action=None): self.path = None self.conn, child_conn = multiprocessing.Pipe() - self.child = multiprocessing.Process(target=external_file_opener, daemon=True, args=(child_conn,)) + self.child = multiprocessing.Process( + target=external_file_opener, + args=(child_conn,) + ) + self.child.daemon = True self.child.start() if path: self.send(path, action) @@ -109,11 +113,11 @@ def send(self, path, action=None): if self.path is not None: raise AttributeError('path may only be set once') self.path = str(path) - self.conn.send( (str(path), action) ) + self.conn.send((str(path), action)) return self.conn.recv() def cleanup(self): - # send a message to the child to exit; it will exit whether the path was sent or not + # send a message to the child to exit if self.child: self.conn.send(True) self.child.join() diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index fc336e23add..88b4d1350e7 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -5,7 +5,11 @@ import pytest from pip._internal.utils.filesystem import copy2_fixed, is_socket -from tests.lib.filesystem import make_socket_file, make_unreadable_file, FileOpener +from tests.lib.filesystem import ( + FileOpener, + make_socket_file, + make_unreadable_file, +) from tests.lib.path import Path @@ -69,14 +73,14 @@ def test_copy2_fixed_raises_appropriate_errors(create, error_type, tmpdir): def test_file_opener_no_file(process): - # The FileOpener cleans up the subprocess even if the parent never sends the path + # FileOpener joins the subprocess even if the parent never sends the path with FileOpener(): pass assert len(process.children()) == 0 def test_file_opener_not_found(tmpdir, process): - # The FileOpener cleans up the subprocess even if the file cannot be opened + # The FileOpener cleans up the subprocess when the file cannot be opened path = tmpdir.joinpath('foo.txt') with FileOpener(path): pass @@ -84,21 +88,24 @@ def test_file_opener_not_found(tmpdir, process): def test_file_opener_normal(tmpdir, process): - # The FileOpener cleans up the subprocess when the file exists; also tests that path may be deferred + # The FileOpener cleans up the subprocess when the file exists path = tmpdir.joinpath('foo.txt') with open(path, 'w') as f: f.write('Hello\n') - with FileOpener() as opener: - opener.send(path) + with FileOpener(path): + pass assert len(process.children()) == 0 @skip_unless_windows def test_file_opener_produces_unlink_error(tmpdir, process): + # FileOpener forces an error on Windows when we attempt to remove a file + # The initial path may be deferred; which must be tested with an error path = tmpdir.joinpath('foo.txt') with open(path, 'w') as f: f.write('Hello\n') - with FileOpener(path): + with FileOpener() as opener: + opener.send(path) with pytest.raises(OSError): os.unlink(path) diff --git a/tests/unit/test_utils_temp_dir.py b/tests/unit/test_utils_temp_dir.py index 5967ff8606b..9db56c54033 100644 --- a/tests/unit/test_utils_temp_dir.py +++ b/tests/unit/test_utils_temp_dir.py @@ -15,7 +15,6 @@ TempDirectory, global_tempdir_manager, ) - from tests.lib.filesystem import FileOpener @@ -223,7 +222,7 @@ def test_temp_dir_warns_if_cannot_clean(caplog): # Capture only at WARNING level and up with caplog.at_level(logging.WARNING, 'pip._internal.utils.temp_dir'): - # open a file within the temporary directory in a sub-process, so it cannot be cleaned up + # open a file within the temporary directory in a sub-process with FileOpener() as opener: subpath = os.path.join(temp_dir_path, 'foo.txt') with open(subpath, 'w') as f: @@ -232,11 +231,11 @@ def test_temp_dir_warns_if_cannot_clean(caplog): # with the file open, attempt to remove the log directory temp_dir.cleanup() - # assert that a WARNING was logged and that it contains a note about a potential virus scanner + # assert that a WARNING was logged about virus scanner assert 'WARNING' in caplog.text assert 'virus scanner' in caplog.text - # Assure that the cleanup was properly retried; this fix did not change retries + # Assure that the cleanup was properly retried duration = time.time() - stime assert duration >= 2.0 From fc5450ff942fae1b924d9a363c11f0ec71d5d2a6 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 13:23:59 -0500 Subject: [PATCH 7/9] Fix #7280 - address Python2.7 issues resulting from PEP 3151 --- tests/lib/filesystem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index 72b532121ad..6962e7cc00e 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -70,7 +70,7 @@ def external_file_opener(conn): lock_action(f) elif action == 'noread': make_unreadable_file(path) - except OSError: + except (IOError, OSError): traceback.print_exc(None, sys.stderr) # Indicate the file is opened From cb1a7876bdaadf067b6e37e74a9ec3ca205eb21a Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 14:22:17 -0500 Subject: [PATCH 8/9] Fix #7280 - another attempt to satisfy older versions of Python - IOError persists as an issue in Python 2.7.17 on win32 --- tests/lib/filesystem.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index 6962e7cc00e..5ccb5a2ab4a 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -70,7 +70,8 @@ def external_file_opener(conn): lock_action(f) elif action == 'noread': make_unreadable_file(path) - except (IOError, OSError): + # IOError *is* OSError in modern Python + except IOError: traceback.print_exc(None, sys.stderr) # Indicate the file is opened From 0fb324bb3ca7d3ca9420e39ca08114a6a6f233a4 Mon Sep 17 00:00:00 2001 From: Dan Davis Date: Thu, 2 Jan 2020 15:21:44 -0500 Subject: [PATCH 9/9] Issue #7280 - Attempt to address failing python2.7 test --- tests/lib/filesystem.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index 5ccb5a2ab4a..79416749e35 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -70,7 +70,9 @@ def external_file_opener(conn): lock_action(f) elif action == 'noread': make_unreadable_file(path) - # IOError *is* OSError in modern Python + # IOError is OSError post PEP 3151 + except OSError: + traceback.print_exc(None, sys.stderr) except IOError: traceback.print_exc(None, sys.stderr)