Fix issue pypa#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 pypa#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.

Author: Dan Davis

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.

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):

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,101 @@ 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)
+            except (OSError, IOError):
+                # IOError is OSError post PEP 3151
+                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)

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)

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)

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".