Fix crash when faulthandler starts initialized (#6598)

Use suggestion in review and use a subplugin so hooks will only be active
if we enable faulthandler ourselves.

Fix #6575

Co-authored-by: Daniel Hahler <git@thequod.de>

changelog/6575.bugfix.rst

@@ -0,0 +1,3 @@
+Fix internal crash when ``faulthandler`` starts initialized
+(for example with ``PYTHONFAULTHANDLER=1`` environment variable set) and ``faulthandler_timeout`` defined
+in the configuration file.

src/_pytest/faulthandler.py

@@ -17,70 +17,92 @@ def pytest_addoption(parser):
 def pytest_configure(config):
     import faulthandler
 
-    # avoid trying to dup sys.stderr if faulthandler is already enabled
-    if faulthandler.is_enabled():
-        return
+    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:
+        from _pytest.warnings import _issue_warning_captured
 
-    stderr_fd_copy = os.dup(_get_stderr_fileno())
-    config.fault_handler_stderr = os.fdopen(stderr_fd_copy, "w")
-    faulthandler.enable(file=config.fault_handler_stderr)
+        # 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:
+            _issue_warning_captured(
+                pytest.PytestConfigWarning(
+                    "faulthandler module enabled before pytest configuration step, "
+                    "'faulthandler_timeout' option ignored"
+                ),
+                config.hook,
+                stacklevel=2,
+            )
 
 
-def _get_stderr_fileno():
-    try:
-        return sys.stderr.fileno()
-    except (AttributeError, io.UnsupportedOperation):
-        # python-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()
+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):
+        import faulthandler
 
-def pytest_unconfigure(config):
-    import faulthandler
+        stderr_fd_copy = os.dup(self._get_stderr_fileno())
+        config.fault_handler_stderr = os.fdopen(stderr_fd_copy, "w")
+        faulthandler.enable(file=config.fault_handler_stderr)
 
-    faulthandler.disable()
-    # close our dup file installed during pytest_configure
-    f = getattr(config, "fault_handler_stderr", None)
-    if f is not None:
+    def pytest_unconfigure(self, config):
+        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
         config.fault_handler_stderr.close()
         del config.fault_handler_stderr
-        faulthandler.enable(file=_get_stderr_fileno())
+        faulthandler.enable(file=self._get_stderr_fileno())
 
+    @staticmethod
+    def _get_stderr_fileno():
+        try:
+            return sys.stderr.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()
 
-@pytest.hookimpl(hookwrapper=True)
-def pytest_runtest_protocol(item):
-    timeout = float(item.config.getini("faulthandler_timeout") or 0.0)
-    if timeout > 0:
+    @staticmethod
+    def get_timeout_config_value(config):
+        return float(config.getini("faulthandler_timeout") or 0.0)
+
+    @pytest.hookimpl(hookwrapper=True)
+    def pytest_runtest_protocol(self, item):
+        timeout = self.get_timeout_config_value(item.config)
+        stderr = item.config.fault_handler_stderr
+        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
+
+    @pytest.hookimpl(tryfirst=True)
+    def pytest_enter_pdb(self):
+        """Cancel any traceback dumping due to timeout before entering pdb.
+        """
         import faulthandler
 
-        stderr = item.config.fault_handler_stderr
-        faulthandler.dump_traceback_later(timeout, file=stderr)
-        try:
-            yield
-        finally:
-            faulthandler.cancel_dump_traceback_later()
-    else:
-        yield
+        faulthandler.cancel_dump_traceback_later()
 
+    @pytest.hookimpl(tryfirst=True)
+    def pytest_exception_interact(self):
+        """Cancel any traceback dumping due to an interactive exception being
+        raised.
+        """
+        import faulthandler
 
-@pytest.hookimpl(tryfirst=True)
-def pytest_enter_pdb():
-    """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():
-    """Cancel any traceback dumping due to an interactive exception being
-    raised.
-    """
-    import faulthandler
-
-    faulthandler.cancel_dump_traceback_later()
+        faulthandler.cancel_dump_traceback_later()

testing/test_faulthandler.py

@@ -88,7 +88,7 @@ def test_cancel_timeout_on_hook(monkeypatch, hook_name):
     exception (pytest-dev/pytest-faulthandler#14).
     """
     import faulthandler
-    from _pytest import faulthandler as plugin_module
+    from _pytest.faulthandler import FaultHandlerHooks
 
     called = []
 
@@ -98,6 +98,35 @@ def test_cancel_timeout_on_hook(monkeypatch, hook_name):
 
     # call our hook explicitly, we can trust that pytest will call the hook
     # for us at the appropriate moment
-    hook_func = getattr(plugin_module, hook_name)
-    hook_func()
+    hook_func = getattr(FaultHandlerHooks, hook_name)
+    hook_func(self=None)
     assert called == [1]
+
+
+@pytest.mark.parametrize("faulthandler_timeout", [0, 2])
+def test_already_initialized(faulthandler_timeout, testdir):
+    """Test for faulthandler being initialized earlier than pytest (#6575)"""
+    testdir.makepyfile(
+        """
+        def test():
+            import faulthandler
+            assert faulthandler.is_enabled()
+    """
+    )
+    result = testdir.run(
+        sys.executable,
+        "-X",
+        "faulthandler",
+        "-mpytest",
+        testdir.tmpdir,
+        "-o",
+        "faulthandler_timeout={}".format(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