diff --git a/news/7280.bugfix b/news/7280.bugfix new file mode 100644 index 00000000000..17ac0cd2f55 --- /dev/null +++ b/news/7280.bugfix @@ -0,0 +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. 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. diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 0be0ff785cd..1f80cb48f56 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,16 @@ def cleanup(self): """ self._deleted = True if os.path.exists(self._path): - rmtree(self._path) + try: + rmtree(self._path) + except OSError as e: + if sys.platform == 'win32' and e.errno == errno.EACCES: + logger.warning( + "%s (virus scanner may be holding it)." + "cannot remove '%s'", + e.strerror, e.filename) + else: + raise class AdjacentTempDirectory(TempDirectory): diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index dc14b323e33..79416749e35 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -1,9 +1,11 @@ """Helpers for filesystem-dependent tests. """ +import multiprocessing import os import socket import subprocess import sys +import traceback from functools import partial from itertools import chain @@ -34,6 +36,103 @@ 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') + # NOTE: action is for future use and may be unused + if action == 'lock': + lock_action(f) + elif action == 'noread': + make_unreadable_file(path) + # IOError is OSError post PEP 3151 + except OSError: + traceback.print_exc(None, sys.stderr) + except IOError: + 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 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. + + 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, action=None): + self.path = None + self.conn, child_conn = multiprocessing.Pipe() + self.child = multiprocessing.Process( + target=external_file_opener, + args=(child_conn,) + ) + self.child.daemon = True + 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 + 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) diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index 3ef814dce4b..88b4d1350e7 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -1,13 +1,23 @@ 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 ( + FileOpener, + make_socket_file, + make_unreadable_file, +) from tests.lib.path import Path +@pytest.fixture() +def process(): + return psutil.Process() + + def make_file(path): Path(path).touch() @@ -27,6 +37,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 +70,53 @@ 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): + # 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 when 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 + path = tmpdir.joinpath('foo.txt') + with open(path, 'w') as f: + f.write('Hello\n') + 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() as opener: + opener.send(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..9db56c54033 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 @@ -12,6 +15,7 @@ TempDirectory, global_tempdir_manager, ) +from tests.lib.filesystem import FileOpener # No need to test symlinked directories on Windows @@ -207,3 +211,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 + 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 about virus scanner + assert 'WARNING' in caplog.text + assert 'virus scanner' in caplog.text + + # Assure that the cleanup was properly retried + 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".