Skip to content

Commit

Permalink
fix: simplify profile data removal
Browse files Browse the repository at this point in the history
  • Loading branch information
tysmith committed Nov 20, 2024
1 parent a3cc57a commit 8e8a11f
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 115 deletions.
6 changes: 3 additions & 3 deletions src/ffpuppet/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 2 additions & 30 deletions src/ffpuppet/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -33,7 +32,6 @@
"certutil_available",
"certutil_find",
"files_in_use",
"onerror",
"prepare_environment",
"wait_on_files",
"warn_open",
Expand Down Expand Up @@ -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,
Expand Down
28 changes: 9 additions & 19 deletions src/ffpuppet/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
21 changes: 7 additions & 14 deletions src/ffpuppet/puppet_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
46 changes: 9 additions & 37 deletions src/ffpuppet/test_ffpuppet.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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"
Expand Down Expand Up @@ -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"])
Expand Down Expand Up @@ -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())
Expand All @@ -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)
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand All @@ -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)
Expand All @@ -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)
Expand Down
14 changes: 6 additions & 8 deletions src/ffpuppet/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
7 changes: 3 additions & 4 deletions src/ffpuppet/test_puppet_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

from pytest import raises

from .helpers import onerror
from .puppet_logger import PuppetLogger


Expand Down Expand Up @@ -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


Expand Down

0 comments on commit 8e8a11f

Please sign in to comment.