From 341d1a43a774b7067fe7c1533dfcaa6eb327b88f Mon Sep 17 00:00:00 2001 From: Tyson Smith Date: Tue, 19 Nov 2024 16:08:49 -0800 Subject: [PATCH] fix: simplify profile data removal --- src/ffpuppet/core.py | 6 +++--- src/ffpuppet/helpers.py | 32 ++---------------------------- src/ffpuppet/profile.py | 28 +++++++++----------------- src/ffpuppet/puppet_logger.py | 21 +++++++------------- src/ffpuppet/test_profile.py | 14 ++++++------- src/ffpuppet/test_puppet_logger.py | 7 +++---- 6 files changed, 30 insertions(+), 78 deletions(-) diff --git a/src/ffpuppet/core.py b/src/ffpuppet/core.py index 95eaf2f..3f2f998 100644 --- a/src/ffpuppet/core.py +++ b/src/ffpuppet/core.py @@ -586,9 +586,9 @@ def close(self, force_close: bool = False) -> None: self._proc_tree = None self._logs.close() self._checks = [] - assert self.profile - self.profile.remove(ignore_errors=force_close) - self.profile = None + if self.profile is not None: + self.profile.remove() + self.profile = None finally: LOG.debug("reason code: %s", r_code.name) self.reason = r_code diff --git a/src/ffpuppet/helpers.py b/src/ffpuppet/helpers.py index 4405480..d44e952 100644 --- a/src/ffpuppet/helpers.py +++ b/src/ffpuppet/helpers.py @@ -6,13 +6,12 @@ from contextlib import suppress from logging import getLogger -from os import W_OK, access, chmod, environ +from os import environ from pathlib import Path from platform import system -from stat import S_IWUSR from subprocess import STDOUT, CalledProcessError, check_output from time import sleep, time -from typing import TYPE_CHECKING, Any, Callable +from typing import TYPE_CHECKING from psutil import Process, process_iter @@ -33,7 +32,6 @@ "certutil_available", "certutil_find", "files_in_use", - "onerror", "prepare_environment", "wait_on_files", "warn_open", @@ -232,32 +230,6 @@ def files_in_use(files: Iterable[Path]) -> Generator[tuple[Path, int, str]]: yield open_file, proc.info["pid"], proc.info["name"] -def onerror( - func: Callable[[str], Any], path: str, _exc_info: Any -) -> None: # pragma: no cover - """ - Error handler for `shutil.rmtree`. - - If the error is due to an access error (read only file) - it attempts to add write permission and then retries. - - If the error is for another reason it re-raises the error. - - Copyright Michael Foord 2004 - Released subject to the BSD License - ref: http://www.voidspace.org.uk/python/recipebook.shtml#utils - - Usage : `shutil.rmtree(path, onerror=onerror)` - """ - if not access(path, W_OK): - # Is the error an access error? - chmod(path, S_IWUSR) - func(path) - else: - # this should only ever be called from an exception context - raise # pylint: disable=misplaced-bare-raise - - def prepare_environment( target_path: Path, sanitizer_log: Path, diff --git a/src/ffpuppet/profile.py b/src/ffpuppet/profile.py index a3fcd83..3ed160d 100644 --- a/src/ffpuppet/profile.py +++ b/src/ffpuppet/profile.py @@ -13,7 +13,7 @@ from time import time from xml.etree import ElementTree -from .helpers import certutil_available, certutil_find, onerror +from .helpers import certutil_available, certutil_find LOG = getLogger(__name__) @@ -53,8 +53,7 @@ def __init__( self._install_cert(cert, certutil_find(browser_bin)) except Exception: if self.path.exists(): - # pylint: disable=deprecated-argument - rmtree(self.path, onerror=onerror) + rmtree(self.path, ignore_errors=True) raise def __enter__(self) -> Profile: @@ -220,27 +219,18 @@ def invalid_prefs(self) -> Path | None: return self.path / "Invalidprefs.js" return None - def remove(self, ignore_errors: bool = True) -> None: + def remove(self) -> None: """Remove the profile from the filesystem. Args: - ignore_errors: Do not raise exception if error is encountered. + None Returns: None """ - if self.path: - try: - # check if path exists to properly support "onerror" - if self.path.exists(): - LOG.debug("removing profile") - # pylint: disable=deprecated-argument - rmtree(self.path, onerror=onerror) - except OSError: + if self.path is not None: + LOG.debug("removing profile") + rmtree(self.path, ignore_errors=True) + if self.path.exists(): LOG.error("Failed to remove profile '%s'", self.path) - # skip raising here instead of passing ignore_errors to rmtree - # this way onerror is always called if there is an error - if not ignore_errors: - raise - finally: - self.path = None + self.path = None diff --git a/src/ffpuppet/puppet_logger.py b/src/ffpuppet/puppet_logger.py index 4ea6c3d..0f0870d 100644 --- a/src/ffpuppet/puppet_logger.py +++ b/src/ffpuppet/puppet_logger.py @@ -15,7 +15,7 @@ from tempfile import NamedTemporaryFile, mkdtemp from typing import IO, TYPE_CHECKING -from .helpers import onerror, warn_open +from .helpers import warn_open if TYPE_CHECKING: from collections.abc import Iterator, KeysView @@ -108,19 +108,12 @@ def clean_up(self, ignore_errors: bool = False) -> None: if not self.closed: self.close() if self.path is not None: - # try multiple attempts to remove data - for attempt in reversed(range(2)): - try: - # check if path exists to properly support "onerror" - if self.path.exists(): - # pylint: disable=deprecated-argument - rmtree(self.path, ignore_errors=ignore_errors, onerror=onerror) - except OSError: - if attempt == 0: - warn_open(self.path) - raise - continue - break + try: + if self.path.exists(): + rmtree(self.path, ignore_errors=ignore_errors) + except OSError: + warn_open(self.path) + raise self._logs.clear() self.path = None diff --git a/src/ffpuppet/test_profile.py b/src/ffpuppet/test_profile.py index bc7a619..4df3bb8 100644 --- a/src/ffpuppet/test_profile.py +++ b/src/ffpuppet/test_profile.py @@ -200,15 +200,13 @@ def test_profile_05(tmp_path): def test_profile_06(mocker, tmp_path): - """test Profile.remove() failure""" - mocker.patch("ffpuppet.profile.rmtree", autospec=True, side_effect=OSError("test")) + """test Profile.remove() fail to remove data directory""" + mocker.patch("ffpuppet.profile.rmtree", autospec=True) with Profile(working_path=str(tmp_path)) as profile: - profile.remove(ignore_errors=True) - with ( - Profile(working_path=str(tmp_path)) as profile, - raises(OSError, match="test"), - ): - profile.remove(ignore_errors=False) + path = profile.path + profile.remove() + assert profile.path is None + assert path.exists() def test_profile_07(mocker, tmp_path): diff --git a/src/ffpuppet/test_puppet_logger.py b/src/ffpuppet/test_puppet_logger.py index b207b35..21bfe87 100644 --- a/src/ffpuppet/test_puppet_logger.py +++ b/src/ffpuppet/test_puppet_logger.py @@ -10,7 +10,6 @@ from pytest import raises -from .helpers import onerror from .puppet_logger import PuppetLogger @@ -240,15 +239,15 @@ def test_puppet_logger_09(mocker, tmp_path): fake_rmtree.side_effect = OSError("test") with raises(OSError): plog.clean_up() - assert fake_rmtree.call_count == 2 - fake_rmtree.assert_called_with(path, ignore_errors=False, onerror=onerror) + assert fake_rmtree.call_count == 1 + fake_rmtree.assert_called_with(path, ignore_errors=False) assert plog.path is not None fake_rmtree.reset_mock() # test with ignore_errors=True fake_rmtree.side_effect = None plog.clean_up(ignore_errors=True) assert fake_rmtree.call_count == 1 - fake_rmtree.assert_called_with(path, ignore_errors=True, onerror=onerror) + fake_rmtree.assert_called_with(path, ignore_errors=True) assert plog.path is None