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

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'",

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

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)

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)

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