From adc0f29b8f8fa8a63e0592c38503855a5b615a29 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 20 Jan 2021 10:05:36 -0300 Subject: [PATCH 1/2] Always handle faulthandler stderr even if already enabled It seems the code that would not install pytest's faulthandler support if it was already enabled is not really needed at all, and even detrimental when using `python -X dev -m pytest` to run Python in "dev" mode. Also simplified the plugin by removing the hook class, now the hooks will always be active so there's no need to delay the hook definitions anymore. Fix #8258 --- changelog/8258.bugfix.rst | 3 + src/_pytest/faulthandler.py | 134 +++++++++++++++-------------------- testing/test_faulthandler.py | 29 +++----- 3 files changed, 70 insertions(+), 96 deletions(-) create mode 100644 changelog/8258.bugfix.rst diff --git a/changelog/8258.bugfix.rst b/changelog/8258.bugfix.rst new file mode 100644 index 000000000..6518ec0b7 --- /dev/null +++ b/changelog/8258.bugfix.rst @@ -0,0 +1,3 @@ +Fixed issue where pytest's ``faulthandler`` support would not dump traceback on crashes +if the :mod:`faulthandler` module was already enabled during pytest startup (using +``python -X dev -m pytest`` for example). diff --git a/src/_pytest/faulthandler.py b/src/_pytest/faulthandler.py index ff673b5b1..9592de82d 100644 --- a/src/_pytest/faulthandler.py +++ b/src/_pytest/faulthandler.py @@ -25,92 +25,72 @@ def pytest_addoption(parser: Parser) -> None: def pytest_configure(config: Config) -> None: import faulthandler - if not faulthandler.is_enabled(): - # faulthhandler is not enabled, so install plugin that does the actual work - # of enabling faulthandler before each test executes. - config.pluginmanager.register(FaultHandlerHooks(), "faulthandler-hooks") - else: - # Do not handle dumping to stderr if faulthandler is already enabled, so warn - # users that the option is being ignored. - timeout = FaultHandlerHooks.get_timeout_config_value(config) - if timeout > 0: - config.issue_config_time_warning( - pytest.PytestConfigWarning( - "faulthandler module enabled before pytest configuration step, " - "'faulthandler_timeout' option ignored" - ), - stacklevel=2, - ) + stderr_fd_copy = os.dup(get_stderr_fileno()) + config._store[fault_handler_stderr_key] = open(stderr_fd_copy, "w") + faulthandler.enable(file=config._store[fault_handler_stderr_key]) -class FaultHandlerHooks: - """Implements hooks that will actually install fault handler before tests execute, - as well as correctly handle pdb and internal errors.""" +def pytest_unconfigure(config: Config) -> None: + import faulthandler - def pytest_configure(self, config: Config) -> None: - import faulthandler - - stderr_fd_copy = os.dup(self._get_stderr_fileno()) - config._store[fault_handler_stderr_key] = open(stderr_fd_copy, "w") - faulthandler.enable(file=config._store[fault_handler_stderr_key]) - - def pytest_unconfigure(self, config: Config) -> None: - import faulthandler - - faulthandler.disable() - # close our dup file installed during pytest_configure - # re-enable the faulthandler, attaching it to the default sys.stderr - # so we can see crashes after pytest has finished, usually during - # garbage collection during interpreter shutdown + faulthandler.disable() + # Close the dup file installed during pytest_configure. + if fault_handler_stderr_key in config._store: config._store[fault_handler_stderr_key].close() del config._store[fault_handler_stderr_key] - faulthandler.enable(file=self._get_stderr_fileno()) + # Re-enable the faulthandler, attaching it to the default sys.stderr + # so we can see crashes after pytest has finished, usually during + # garbage collection during interpreter shutdown. + faulthandler.enable(file=get_stderr_fileno()) - @staticmethod - def _get_stderr_fileno(): + +def get_stderr_fileno() -> int: + try: + fileno = sys.stderr.fileno() + # The Twisted Logger will return an invalid file descriptor since it is not backed + # by an FD. So, let's also forward this to the same code path as with pytest-xdist. + if fileno == -1: + raise AttributeError() + return fileno + except (AttributeError, io.UnsupportedOperation): + # pytest-xdist monkeypatches sys.stderr with an object that is not an actual file. + # https://docs.python.org/3/library/faulthandler.html#issue-with-file-descriptors + # This is potentially dangerous, but the best we can do. + return sys.__stderr__.fileno() + + +def get_timeout_config_value(config: Config) -> float: + return float(config.getini("faulthandler_timeout") or 0.0) + + +@pytest.hookimpl(hookwrapper=True, trylast=True) +def pytest_runtest_protocol(item: Item) -> Generator[None, None, None]: + timeout = get_timeout_config_value(item.config) + stderr = item.config._store[fault_handler_stderr_key] + if timeout > 0 and stderr is not None: + import faulthandler + + faulthandler.dump_traceback_later(timeout, file=stderr) try: - fileno = sys.stderr.fileno() - # The Twisted Logger will return an invalid file descriptor since it is not backed - # by an FD. So, let's also forward this to the same code path as with pytest-xdist. - if fileno == -1: - raise AttributeError() - return fileno - except (AttributeError, io.UnsupportedOperation): - # pytest-xdist monkeypatches sys.stderr with an object that is not an actual file. - # https://docs.python.org/3/library/faulthandler.html#issue-with-file-descriptors - # This is potentially dangerous, but the best we can do. - return sys.__stderr__.fileno() - - @staticmethod - def get_timeout_config_value(config): - return float(config.getini("faulthandler_timeout") or 0.0) - - @pytest.hookimpl(hookwrapper=True, trylast=True) - def pytest_runtest_protocol(self, item: Item) -> Generator[None, None, None]: - timeout = self.get_timeout_config_value(item.config) - stderr = item.config._store[fault_handler_stderr_key] - if timeout > 0 and stderr is not None: - import faulthandler - - faulthandler.dump_traceback_later(timeout, file=stderr) - try: - yield - finally: - faulthandler.cancel_dump_traceback_later() - else: yield + finally: + faulthandler.cancel_dump_traceback_later() + else: + yield - @pytest.hookimpl(tryfirst=True) - def pytest_enter_pdb(self) -> None: - """Cancel any traceback dumping due to timeout before entering pdb.""" - import faulthandler - faulthandler.cancel_dump_traceback_later() +@pytest.hookimpl(tryfirst=True) +def pytest_enter_pdb() -> None: + """Cancel any traceback dumping due to timeout before entering pdb.""" + import faulthandler - @pytest.hookimpl(tryfirst=True) - def pytest_exception_interact(self) -> None: - """Cancel any traceback dumping due to an interactive exception being - raised.""" - import faulthandler + faulthandler.cancel_dump_traceback_later() - faulthandler.cancel_dump_traceback_later() + +@pytest.hookimpl(tryfirst=True) +def pytest_exception_interact() -> None: + """Cancel any traceback dumping due to an interactive exception being + raised.""" + import faulthandler + + faulthandler.cancel_dump_traceback_later() diff --git a/testing/test_faulthandler.py b/testing/test_faulthandler.py index 370084c12..411e841a3 100644 --- a/testing/test_faulthandler.py +++ b/testing/test_faulthandler.py @@ -94,7 +94,7 @@ def test_cancel_timeout_on_hook(monkeypatch, hook_name) -> None: to timeout before entering pdb (pytest-dev/pytest-faulthandler#12) or any other interactive exception (pytest-dev/pytest-faulthandler#14).""" import faulthandler - from _pytest.faulthandler import FaultHandlerHooks + from _pytest import faulthandler as faulthandler_plugin called = [] @@ -104,19 +104,18 @@ def test_cancel_timeout_on_hook(monkeypatch, hook_name) -> None: # call our hook explicitly, we can trust that pytest will call the hook # for us at the appropriate moment - hook_func = getattr(FaultHandlerHooks, hook_name) - hook_func(self=None) + hook_func = getattr(faulthandler_plugin, hook_name) + hook_func() assert called == [1] -@pytest.mark.parametrize("faulthandler_timeout", [0, 2]) -def test_already_initialized(faulthandler_timeout: int, pytester: Pytester) -> None: - """Test for faulthandler being initialized earlier than pytest (#6575).""" +def test_already_initialized_crash(pytester: Pytester) -> None: + """Even if faulthandler is already initialized, we still dump tracebacks on crashes (#8258).""" pytester.makepyfile( """ def test(): import faulthandler - assert faulthandler.is_enabled() + faulthandler._sigabrt() """ ) result = pytester.run( @@ -125,22 +124,14 @@ def test_already_initialized(faulthandler_timeout: int, pytester: Pytester) -> N "faulthandler", "-mpytest", pytester.path, - "-o", - f"faulthandler_timeout={faulthandler_timeout}", ) - # ensure warning is emitted if faulthandler_timeout is configured - warning_line = "*faulthandler.py*faulthandler module enabled before*" - if faulthandler_timeout > 0: - result.stdout.fnmatch_lines(warning_line) - else: - result.stdout.no_fnmatch_line(warning_line) - result.stdout.fnmatch_lines("*1 passed*") - assert result.ret == 0 + result.stderr.fnmatch_lines(["*Fatal Python error*"]) + assert result.ret != 0 def test_get_stderr_fileno_invalid_fd() -> None: """Test for faulthandler being able to handle invalid file descriptors for stderr (#8249).""" - from _pytest.faulthandler import FaultHandlerHooks + from _pytest.faulthandler import get_stderr_fileno class StdErrWrapper(io.StringIO): """ @@ -159,4 +150,4 @@ def test_get_stderr_fileno_invalid_fd() -> None: # Even when the stderr wrapper signals an invalid file descriptor, # ``_get_stderr_fileno()`` should return the real one. - assert FaultHandlerHooks._get_stderr_fileno() == 2 + assert get_stderr_fileno() == 2 From 33861098d9145d3441c317c9d9b2265654b53b05 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 25 Jan 2021 12:28:00 -0300 Subject: [PATCH 2/2] Only re-enable fauthandler during unconfigure if it was enabled before --- src/_pytest/faulthandler.py | 9 +++++---- testing/test_faulthandler.py | 37 +++++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/_pytest/faulthandler.py b/src/_pytest/faulthandler.py index 9592de82d..c8eb03101 100644 --- a/src/_pytest/faulthandler.py +++ b/src/_pytest/faulthandler.py @@ -12,6 +12,7 @@ from _pytest.store import StoreKey fault_handler_stderr_key = StoreKey[TextIO]() +fault_handler_originally_enabled_key = StoreKey[bool]() def pytest_addoption(parser: Parser) -> None: @@ -27,6 +28,7 @@ def pytest_configure(config: Config) -> None: stderr_fd_copy = os.dup(get_stderr_fileno()) config._store[fault_handler_stderr_key] = open(stderr_fd_copy, "w") + config._store[fault_handler_originally_enabled_key] = faulthandler.is_enabled() faulthandler.enable(file=config._store[fault_handler_stderr_key]) @@ -38,10 +40,9 @@ def pytest_unconfigure(config: Config) -> None: if fault_handler_stderr_key in config._store: config._store[fault_handler_stderr_key].close() del config._store[fault_handler_stderr_key] - # Re-enable the faulthandler, attaching it to the default sys.stderr - # so we can see crashes after pytest has finished, usually during - # garbage collection during interpreter shutdown. - faulthandler.enable(file=get_stderr_fileno()) + if config._store.get(fault_handler_originally_enabled_key, False): + # Re-enable the faulthandler if it was originally enabled. + faulthandler.enable(file=get_stderr_fileno()) def get_stderr_fileno() -> int: diff --git a/testing/test_faulthandler.py b/testing/test_faulthandler.py index 411e841a3..5b7911f21 100644 --- a/testing/test_faulthandler.py +++ b/testing/test_faulthandler.py @@ -19,22 +19,41 @@ def test_enabled(pytester: Pytester) -> None: assert result.ret != 0 -def test_crash_near_exit(pytester: Pytester) -> None: - """Test that fault handler displays crashes that happen even after - pytest is exiting (for example, when the interpreter is shutting down).""" +def setup_crashing_test(pytester: Pytester) -> None: pytester.makepyfile( """ - import faulthandler - import atexit - def test_ok(): - atexit.register(faulthandler._sigabrt) - """ + import faulthandler + import atexit + def test_ok(): + atexit.register(faulthandler._sigabrt) + """ ) - result = pytester.runpytest_subprocess() + + +def test_crash_during_shutdown_captured(pytester: Pytester) -> None: + """ + Re-enable faulthandler if pytest encountered it enabled during configure. + We should be able to then see crashes during interpreter shutdown. + """ + setup_crashing_test(pytester) + args = (sys.executable, "-Xfaulthandler", "-mpytest") + result = pytester.run(*args) result.stderr.fnmatch_lines(["*Fatal Python error*"]) assert result.ret != 0 +def test_crash_during_shutdown_not_captured(pytester: Pytester) -> None: + """ + Check that pytest leaves faulthandler disabled if it was not enabled during configure. + This prevents us from seeing crashes during interpreter shutdown (see #8260). + """ + setup_crashing_test(pytester) + args = (sys.executable, "-mpytest") + result = pytester.run(*args) + result.stderr.no_fnmatch_line("*Fatal Python error*") + assert result.ret != 0 + + def test_disabled(pytester: Pytester) -> None: """Test option to disable fault handler in the command line.""" pytester.makepyfile(