From 660dafb37f7ea3775230fccd4d483f73b8769560 Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Mon, 9 Dec 2019 16:59:36 +0100 Subject: [PATCH 1/5] Ignore errors in temporary directory cleanup pip should not exit with an error when it fails to cleanup temporary files after it has already successfully installed packages. --- src/pip/_internal/utils/misc.py | 40 +++++++++++++++++++++++------ src/pip/_internal/utils/temp_dir.py | 34 ++++++++++++++++++++++-- tests/unit/test_utils.py | 10 +++++--- tests/unit/test_utils_temp_dir.py | 23 +++++++++++++++++ 4 files changed, 94 insertions(+), 13 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index bd191c4e14f..f366aa4053b 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -11,6 +11,7 @@ import sys import sysconfig import urllib.parse +from functools import partial from io import StringIO from itertools import filterfalse, tee, zip_longest from types import TracebackType @@ -123,15 +124,35 @@ def get_prog() -> str: # Retry every half second for up to 3 seconds # Tenacity raises RetryError by default, explicitly raise the original exception @retry(reraise=True, stop=stop_after_delay(3), wait=wait_fixed(0.5)) -def rmtree(dir: str, ignore_errors: bool = False) -> None: +def rmtree( + dir: str, + ignore_errors: bool = False, + onexc: Optional[Callable[[Any, Any, Any], Any]] = None, +) -> None: + if ignore_errors: + onexc = _onerror_ignore + elif onexc is None: + onexc = _onerror_reraise if sys.version_info >= (3, 12): - shutil.rmtree(dir, ignore_errors=ignore_errors, onexc=rmtree_errorhandler) + shutil.rmtree(dir, onexc=partial(rmtree_errorhandler, onexc=onexc)) else: - shutil.rmtree(dir, ignore_errors=ignore_errors, onerror=rmtree_errorhandler) + shutil.rmtree(dir, onerror=partial(rmtree_errorhandler, onexc=onexc)) + + +def _onerror_ignore(*_args: Any) -> None: + pass + + +def _onerror_reraise(*_args: Any) -> None: + raise def rmtree_errorhandler( - func: Callable[..., Any], path: str, exc_info: Union[ExcInfo, BaseException] + func: Callable[..., Any], + path: str, + exc_info: Union[ExcInfo, BaseException], + *, + onexc: Callable[..., Any] = _onerror_reraise, ) -> None: """On Windows, the files in .svn are read-only, so when rmtree() tries to remove them, an exception is thrown. We catch that here, remove the @@ -146,10 +167,13 @@ def rmtree_errorhandler( # convert to read/write os.chmod(path, stat.S_IWRITE) # use the original function to repeat the operation - func(path) - return - else: - raise + try: + func(path) + return + except OSError: + pass + + onexc(func, path, exc_info) def display_path(path: str) -> str: diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 8ee8a1cb180..7d3960734cf 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -3,8 +3,9 @@ import logging import os.path import tempfile +import traceback from contextlib import ExitStack, contextmanager -from typing import Any, Dict, Generator, Optional, TypeVar, Union +from typing import Any, Callable, Dict, Generator, Optional, Tuple, Type, TypeVar, Union from pip._internal.utils.misc import enum, rmtree @@ -106,6 +107,7 @@ def __init__( delete: Union[bool, None, _Default] = _default, kind: str = "temp", globally_managed: bool = False, + ignore_cleanup_errors: bool = True, ): super().__init__() @@ -128,6 +130,7 @@ def __init__( self._deleted = False self.delete = delete self.kind = kind + self.ignore_cleanup_errors = ignore_cleanup_errors if globally_managed: assert _tempdir_manager is not None @@ -170,7 +173,34 @@ def cleanup(self) -> None: self._deleted = True if not os.path.exists(self._path): return - rmtree(self._path) + + def onerror( + func: Callable[[str], Any], + path: str, + exc_info: Tuple[Type[BaseException], BaseException, Any], + ) -> None: + """Log a warning for a `rmtree` error and continue""" + exc_val = "\n".join(traceback.format_exception_only(*exc_info[:2])) + exc_val = exc_val.rstrip() # remove trailing new line + if func in (os.unlink, os.remove, os.rmdir): + logging.warning( + "Failed to remove a temporary file '%s' due to %s.\n" + "You can safely remove it manually.", + path, + exc_val, + ) + else: + logging.warning("%s failed with %s.", func.__qualname__, exc_val) + + if self.ignore_cleanup_errors: + try: + # first try with tenacity; retrying to handle ephemeral errors + rmtree(self._path, ignore_errors=False) + except OSError: + # last pass ignore/log all errors + rmtree(self._path, onexc=onerror) + else: + rmtree(self._path) class AdjacentTempDirectory(TempDirectory): diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 450081cfd03..d3b0d32d12f 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -257,9 +257,13 @@ def test_rmtree_errorhandler_reraises_error(tmpdir: Path) -> None: except RuntimeError: # Make sure the handler reraises an exception with pytest.raises(RuntimeError, match="test message"): - # Argument 3 to "rmtree_errorhandler" has incompatible type "None"; expected - # "Tuple[Type[BaseException], BaseException, TracebackType]" - rmtree_errorhandler(mock_func, path, None) # type: ignore[arg-type] + # Argument 3 to "rmtree_errorhandler" has incompatible type + # "Union[Tuple[Type[BaseException], BaseException, TracebackType], + # Tuple[None, None, None]]"; expected "Tuple[Type[BaseException], + # BaseException, TracebackType]" + rmtree_errorhandler( + mock_func, path, sys.exc_info() # type: ignore[arg-type] + ) mock_func.assert_not_called() diff --git a/tests/unit/test_utils_temp_dir.py b/tests/unit/test_utils_temp_dir.py index 4a656d23ace..a6cd0d0e5af 100644 --- a/tests/unit/test_utils_temp_dir.py +++ b/tests/unit/test_utils_temp_dir.py @@ -4,6 +4,7 @@ import tempfile from pathlib import Path from typing import Any, Iterator, Optional, Union +from unittest import mock import pytest @@ -274,3 +275,25 @@ def test_tempdir_registry_lazy(should_delete: bool) -> None: registry.set_delete("test-for-lazy", should_delete) assert os.path.exists(path) assert os.path.exists(path) == (not should_delete) + + +def test_tempdir_cleanup_ignore_errors() -> None: + os_unlink = os.unlink + + # mock os.unlink to fail with EACCES for a specific filename to simulate + # how removing a loaded exe/dll behaves. + def unlink(name: str, *args: Any, **kwargs: Any) -> None: + if "bomb" in name: + raise PermissionError(name) + else: + os_unlink(name) + + with mock.patch("os.unlink", unlink): + with TempDirectory(ignore_cleanup_errors=True) as tmp_dir: + path = tmp_dir.path + with open(os.path.join(path, "bomb"), "a"): + pass + + filename = os.path.join(path, "bomb") + assert os.path.isfile(filename) + os.unlink(filename) From 4ff65abdf7a0f22ab0200993342168bf96aff8b9 Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Fri, 19 Aug 2022 14:25:59 +0200 Subject: [PATCH 2/5] Fix 'force' remove file without write permissions Preserve existing mode flags, handle case where we even lack permission to change the mode. --- src/pip/_internal/utils/misc.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index f366aa4053b..b7b32f0f8cf 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -154,24 +154,34 @@ def rmtree_errorhandler( *, onexc: Callable[..., Any] = _onerror_reraise, ) -> None: - """On Windows, the files in .svn are read-only, so when rmtree() tries to - remove them, an exception is thrown. We catch that here, remove the - read-only attribute, and hopefully continue without problems.""" + """ + `rmtree` error handler to 'force' a file remove (i.e. like `rm -f`). + + * If a file is readonly then it's write flag is set and operation is + retried. + + * `onerror` is the original callback from `rmtree(... onerror=onerror)` + that is chained at the end if the "rm -f" still fails. + """ try: - has_attr_readonly = not (os.stat(path).st_mode & stat.S_IWRITE) + st_mode = os.stat(path).st_mode except OSError: # it's equivalent to os.path.exists return - if has_attr_readonly: + if not st_mode & stat.S_IWRITE: # convert to read/write - os.chmod(path, stat.S_IWRITE) - # use the original function to repeat the operation try: - func(path) - return + os.chmod(path, st_mode | stat.S_IWRITE) except OSError: pass + else: + # use the original function to repeat the operation + try: + func(path) + return + except OSError: + pass onexc(func, path, exc_info) From dddf4f829308536211107c691c83e75c1f9bacf5 Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Mon, 22 Aug 2022 12:37:23 +0200 Subject: [PATCH 3/5] Add a news entry --- news/11394.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/11394.bugfix.rst diff --git a/news/11394.bugfix.rst b/news/11394.bugfix.rst new file mode 100644 index 00000000000..9f2501db46c --- /dev/null +++ b/news/11394.bugfix.rst @@ -0,0 +1 @@ +Ignore errors in temporary directory cleanup (show a warning instead). From 6a9098ed48f421be549059b5dd3a92325f1bb37b Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Wed, 7 Sep 2022 12:07:28 +0200 Subject: [PATCH 4/5] Show a single warning on temp directory cleanup Log individual errors at debug logging level. --- src/pip/_internal/utils/temp_dir.py | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 7d3960734cf..ae8591ec83d 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -5,7 +5,18 @@ import tempfile import traceback from contextlib import ExitStack, contextmanager -from typing import Any, Callable, Dict, Generator, Optional, Tuple, Type, TypeVar, Union +from typing import ( + Any, + Callable, + Dict, + Generator, + List, + Optional, + Tuple, + Type, + TypeVar, + Union, +) from pip._internal.utils.misc import enum, rmtree @@ -174,6 +185,8 @@ def cleanup(self) -> None: if not os.path.exists(self._path): return + errors: List[BaseException] = [] + def onerror( func: Callable[[str], Any], path: str, @@ -183,14 +196,14 @@ def onerror( exc_val = "\n".join(traceback.format_exception_only(*exc_info[:2])) exc_val = exc_val.rstrip() # remove trailing new line if func in (os.unlink, os.remove, os.rmdir): - logging.warning( - "Failed to remove a temporary file '%s' due to %s.\n" - "You can safely remove it manually.", + logger.debug( + "Failed to remove a temporary file '%s' due to %s.\n", path, exc_val, ) else: - logging.warning("%s failed with %s.", func.__qualname__, exc_val) + logger.debug("%s failed with %s.", func.__qualname__, exc_val) + errors.append(exc_info[1]) if self.ignore_cleanup_errors: try: @@ -199,6 +212,12 @@ def onerror( except OSError: # last pass ignore/log all errors rmtree(self._path, onexc=onerror) + if errors: + logger.warning( + "Failed to remove contents of a temporary directory '%s'.\n" + "You can safely remove it manually.", + self._path, + ) else: rmtree(self._path) From 2928750ae60dba841d22b398c779a099d1fdf3ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Erjavec?= Date: Wed, 7 Sep 2022 14:45:27 +0200 Subject: [PATCH 5/5] Change warning wording Co-authored-by: Tzu-ping Chung --- src/pip/_internal/utils/temp_dir.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index ae8591ec83d..99d1ba834ef 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -214,7 +214,7 @@ def onerror( rmtree(self._path, onexc=onerror) if errors: logger.warning( - "Failed to remove contents of a temporary directory '%s'.\n" + "Failed to remove contents in a temporary directory '%s'.\n" "You can safely remove it manually.", self._path, )