Merge pull request #8260 from nicoddemus/faulthandler-mode-X-8258

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).

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:
@@ -25,49 +26,26 @@ def pytest_addoption(parser: Parser) -> None:
 def pytest_configure(config: Config) -> None:
     import faulthandler
 
-    if not faulthandler.is_enabled():
+    stderr_fd_copy = os.dup(get_stderr_fileno())
-        # 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,
-            )
-
-
-class FaultHandlerHooks:
-    """Implements hooks that will actually install fault handler before tests execute,
-    as well as correctly handle pdb and internal errors."""
-
-    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")
+    config._store[fault_handler_originally_enabled_key] = faulthandler.is_enabled()
     faulthandler.enable(file=config._store[fault_handler_stderr_key])
 
-    def pytest_unconfigure(self, config: Config) -> None:
+
+def pytest_unconfigure(config: Config) -> None:
     import faulthandler
 
     faulthandler.disable()
-        # close our dup file installed during pytest_configure
+    # Close the dup file installed during pytest_configure.
-        # re-enable the faulthandler, attaching it to the default sys.stderr
+    if fault_handler_stderr_key in config._store:
-        # so we can see crashes after pytest has finished, usually during
-        # garbage collection during interpreter shutdown
         config._store[fault_handler_stderr_key].close()
         del config._store[fault_handler_stderr_key]
-        faulthandler.enable(file=self._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())
 
-    @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
@@ -81,13 +59,14 @@ class FaultHandlerHooks:
         # This is potentially dangerous, but the best we can do.
         return sys.__stderr__.fileno()
 
-    @staticmethod
+
-    def get_timeout_config_value(config):
+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(self, item: Item) -> Generator[None, None, None]:
+@pytest.hookimpl(hookwrapper=True, trylast=True)
-        timeout = self.get_timeout_config_value(item.config)
+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
@@ -100,15 +79,17 @@ class FaultHandlerHooks:
     else:
         yield
 
-    @pytest.hookimpl(tryfirst=True)
+
-    def pytest_enter_pdb(self) -> None:
+@pytest.hookimpl(tryfirst=True)
+def pytest_enter_pdb() -> None:
     """Cancel any traceback dumping due to timeout before entering pdb."""
     import faulthandler
 
     faulthandler.cancel_dump_traceback_later()
 
-    @pytest.hookimpl(tryfirst=True)
+
-    def pytest_exception_interact(self) -> None:
+@pytest.hookimpl(tryfirst=True)
+def pytest_exception_interact() -> None:
     """Cancel any traceback dumping due to an interactive exception being
     raised."""
     import faulthandler

testing/test_faulthandler.py

@@ -19,9 +19,7 @@ def test_enabled(pytester: Pytester) -> None:
     assert result.ret != 0
 
 
-def test_crash_near_exit(pytester: Pytester) -> None:
+def setup_crashing_test(pytester: Pytester) -> None:
-    """Test that fault handler displays crashes that happen even after
-    pytest is exiting (for example, when the interpreter is shutting down)."""
     pytester.makepyfile(
         """
         import faulthandler
@@ -30,11 +28,32 @@ def test_crash_near_exit(pytester: Pytester) -> None:
             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(
@@ -94,7 +113,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 +123,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 = getattr(faulthandler_plugin, hook_name)
-    hook_func(self=None)
+    hook_func()
     assert called == [1]
 
 
-@pytest.mark.parametrize("faulthandler_timeout", [0, 2])
+def test_already_initialized_crash(pytester: Pytester) -> None:
-def test_already_initialized(faulthandler_timeout: int, pytester: Pytester) -> None:
+    """Even if faulthandler is already initialized, we still dump tracebacks on crashes (#8258)."""
-    """Test for faulthandler being initialized earlier than pytest (#6575)."""
     pytester.makepyfile(
         """
         def test():
             import faulthandler
-            assert faulthandler.is_enabled()
+            faulthandler._sigabrt()
     """
     )
     result = pytester.run(
@@ -125,22 +143,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
+    result.stderr.fnmatch_lines(["*Fatal Python error*"])
-    warning_line = "*faulthandler.py*faulthandler module enabled before*"
+    assert result.ret != 0
-    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
 
 
 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 +169,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