From 8e8a11fe0aaf2d2d585a5de32a1f132317a5e082 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_ffpuppet.py | 46 ++++++------------------------ src/ffpuppet/test_profile.py | 14 ++++----- src/ffpuppet/test_puppet_logger.py | 7 ++--- 7 files changed, 39 insertions(+), 115 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_ffpuppet.py b/src/ffpuppet/test_ffpuppet.py index 4f705bf..aebbde3 100644 --- a/src/ffpuppet/test_ffpuppet.py +++ b/src/ffpuppet/test_ffpuppet.py @@ -590,34 +590,6 @@ def _fake_create_log(src, filename, _timeout: int = 90): def test_ffpuppet_23(tmp_path): - """test rmtree error handler""" - # normal profile creation - # - just create a puppet, write a readonly file in its profile, then call close() - with FFPuppet() as ffp: - ffp.launch(TESTFF_BIN) - assert ffp.profile is not None - ro_file = ffp.profile.path / "read-only-test.txt" - ro_file.touch() - ro_file.chmod(S_IREAD) - ffp.close() - assert not ro_file.is_file() - ffp.clean_up() - # use template profile that contains a readonly file - profile = tmp_path / "profile" - profile.mkdir() - ro_file = profile / "read-only.txt" - ro_file.touch() - ro_file.chmod(S_IREAD) - with FFPuppet(use_profile=profile) as ffp: - ffp.launch(TESTFF_BIN) - assert ffp.profile is not None - prof_path = ffp.profile.path - assert prof_path.is_dir() - ffp.close() - assert not prof_path.is_dir() - - -def test_ffpuppet_24(tmp_path): """test using a readonly prefs.js and extension""" prefs = tmp_path / "prefs.js" prefs.touch() @@ -633,7 +605,7 @@ def test_ffpuppet_24(tmp_path): assert not prof_path.is_dir() -def test_ffpuppet_25(mocker, tmp_path): +def test_ffpuppet_24(mocker, tmp_path): """test _crashreports()""" mocker.patch( "ffpuppet.core.check_output", autospec=True, return_value=b"valgrind-99.0" @@ -695,7 +667,7 @@ def close(self, force_close=False): assert not ffp._logs.watching -def test_ffpuppet_26(mocker, tmp_path): +def test_ffpuppet_25(mocker, tmp_path): """test build_launch_cmd()""" with FFPuppet() as ffp: cmd = ffp.build_launch_cmd("bin_path", ["test"]) @@ -743,7 +715,7 @@ def test_ffpuppet_26(mocker, tmp_path): assert cmd[0] == "valgrind" -def test_ffpuppet_27(): +def test_ffpuppet_26(): """test cpu_usage()""" with FFPuppet() as ffp: assert not any(ffp.cpu_usage()) @@ -756,7 +728,7 @@ def test_ffpuppet_27(): assert ffp.wait(timeout=10) -def test_ffpuppet_28(mocker): +def test_ffpuppet_27(mocker): """test _dbg_sanity_check()""" fake_system = mocker.patch("ffpuppet.core.system", autospec=True) fake_chkout = mocker.patch("ffpuppet.core.check_output", autospec=True) @@ -815,7 +787,7 @@ def test_ffpuppet_28(mocker): FFPuppet._dbg_sanity_check(Debugger.VALGRIND) -def test_ffpuppet_29(mocker, tmp_path): +def test_ffpuppet_28(mocker, tmp_path): """test FFPuppet.close() setting reason""" class StubbedProc(FFPuppet): @@ -914,7 +886,7 @@ def launch(self): assert fake_wait_files.call_count == 0 -def test_ffpuppet_30(): +def test_ffpuppet_29(): """test ignoring benign sanitizer logs""" with FFPuppet() as ffp: ffp.launch(TESTFF_BIN) @@ -939,7 +911,7 @@ def test_ffpuppet_30(): (False, FileNotFoundError), ], ) -def test_ffpuppet_31(mocker, tmp_path, bin_exists, expect_exc): +def test_ffpuppet_30(mocker, tmp_path, bin_exists, expect_exc): """test Popen failure during launch""" bin_fake = tmp_path / "fake_bin" if bin_exists: @@ -957,7 +929,7 @@ def test_ffpuppet_31(mocker, tmp_path, bin_exists, expect_exc): @mark.skipif(system() != "Windows", reason="Only supported on Windows") -def test_ffpuppet_32(mocker): +def test_ffpuppet_31(mocker): """test FFPuppet.launch() config_job_object code path""" mocker.patch("ffpuppet.core.ProcessTree", autospec=True) fake_bts = mocker.patch("ffpuppet.core.Bootstrapper", autospec=True) @@ -977,7 +949,7 @@ def test_ffpuppet_32(mocker): assert resume_suspended.mock_calls[0] == mocker.call(789) -def test_ffpuppet_33(mocker): +def test_ffpuppet_32(mocker): """test FFPuppet.dump_coverage()""" with FFPuppet() as ffp: ffp._proc_tree = mocker.Mock(spec_set=ProcessTree) 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