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

@@ -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

@@ -25,49 +25,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")
     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())
+    # 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
@@ -81,13 +58,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]:
+def pytest_runtest_protocol(item: Item) -> Generator[None, None, None]:
-        timeout = self.get_timeout_config_value(item.config)
+    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 +78,17 @@ class FaultHandlerHooks:
     else:
         yield
 
+
 @pytest.hookimpl(tryfirst=True)
-    def pytest_enter_pdb(self) -> None:
+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:
+def pytest_exception_interact() -> None:
     """Cancel any traceback dumping due to an interactive exception being
     raised."""
     import faulthandler

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 = 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 +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
+    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 +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