Add capture parameter to CliRunner and revert default-mode fileno exposure

Closes: #3384
Refs: #3244

Author: kdeldycke

CHANGES.rst

@@ -49,6 +49,18 @@ Unreleased
     :issue:`3043`
 -   Add :class:`NoSuchCommand` exception with suggestions for misspelled
     commands. :issue:`3107` :pr:`3228`
+-   Add ``capture`` parameter to :class:`CliRunner` with two modes: ``sys``
+    (default) and ``fd``. ``fd`` redirects file descriptors ``1`` and ``2``
+    via :func:`os.dup2` so output that bypasses ``sys.stdout`` (stale stream
+    references, C extensions, subprocesses, ``faulthandler``) is captured
+    with proper isolation. :issue:`854` :issue:`2412` :issue:`2468`
+    :issue:`2497` :issue:`2761` :issue:`2827` :issue:`2865`
+-   Revert the ``8.3.3`` change that exposed the original file descriptor
+    via ``fileno()`` on the redirected ``CliRunner`` streams in the default
+    capture mode. ``os.dup2(w, sys.stdout.fileno())`` calls inside a CLI no
+    longer mutate the host runner's stdout, which broke Pytest's ``fd``-level
+    capture teardown. C-level consumers that need a real ``fd`` should use
+    ``capture="fd"``. :issue:`3384` :pr:`3391`
 
 Version 8.3.3
 -------------

docs/testing.md

@@ -204,50 +204,62 @@ Prompts will be emulated so they write the input data to
 the output stream as well. If hidden input is expected then this
 does not happen.
 
-## File Descriptors and Low-Level I/O
-
-{class}`CliRunner` captures output by replacing
-`sys.stdout` and `sys.stderr` with in-memory
-{class}`~io.BytesIO`-backed wrappers. This is
-Python-level redirection: calls to {func}`~click.echo`,
-{func}`print`, or `sys.stdout.write()` are captured, but
-the wrappers have no OS-level file descriptor.
-
-Code that calls `fileno()` on `sys.stdout` or
-`sys.stderr`, like {mod}`faulthandler`,
-{mod}`subprocess`, or C extensions, would normally crash
-with {exc}`io.UnsupportedOperation` inside
-{class}`CliRunner`.
-
-To avoid this, {class}`CliRunner` preserves the original
-stream's file descriptor and exposes it via `fileno()` on
-the replacement wrapper.
-
-This means:
-- **Python-level writes** (`print()`, `click.echo()`,
-  ...) are captured as usual.
-- **fd-level writes** (C code writing directly to the
-  file descriptor) go to the original terminal and are
-  **not** captured.
-
-This is the same trade-off that
-[pytest](https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html)
-makes with its two capture modes:
-
-- `capsys`, which captures Python-level output, where
-  `fileno()` raises `UnsupportedOperation` and fd-level
-  writes are not captured.
-- `capfd`, which captures fd-level output via
-  `os.dup2()`, where `fileno()` works and fd-level
-  writes *are* captured.
-
-Rather than implementing a full `capfd`-style mechanism,
-{class}`CliRunner` takes the simpler path: expose the
-original `fd` so that standard library helpers keep
-working, while accepting that their output is not
-captured.
-
-```{versionchanged} 8.3.3
-`fileno()` on the redirected streams now returns the
-original stream's file descriptor instead of raising.
+## Capture modes
+
+{class}`CliRunner` captures output by replacing `sys.stdout` and `sys.stderr`
+with in-memory wrappers. The `capture` parameter controls which strategy is
+used.
+
+### `capture="sys"` (default)
+
+Captures Python-level writes (`print()`, `click.echo()`, `sys.stdout.write()`).
+It is fast and sufficient for most Click applications.
+
+Code that holds a reference to the original `sys.stdout` (like a library that
+does `from sys import stdout` at import time) bypasses the capture and its
+output is lost.
+
+In this mode `sys.stdout.fileno()` and `sys.stderr.fileno()` raise
+{exc}`io.UnsupportedOperation`, matching the pre-`8.3.3` behavior. C-level
+consumers ({mod}`faulthandler`, {mod}`subprocess`, C extensions) that expect a
+real file descriptor must opt into the `capture="fd"` mode.
+
+### `capture="fd"`
+
+Redirects OS file descriptors `1` and `2` to a temporary file via
+{func}`os.dup2`, inspired by [Pytest's
+`capfd`](https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html).
+This catches output that bypasses `sys.stdout`, including:
+
+- Stale references to the original `sys.stdout` and `sys.stderr`.
+- Logging frameworks that cache the original stream (like `structlog` or the
+  stdlib's `logging` module).
+- C extensions and subprocesses that write directly to `fd 1` or `fd 2`.
+
+```python
+from click.testing import CliRunner
+from myapp import cli
+
+
+def test_captures_everything():
+    runner = CliRunner(capture="fd")
+    result = runner.invoke(cli)
+    # result.stdout contains both Python-level and fd-level output
+    assert "expected output" in result.stdout
+```
+
+In this mode `sys.stdout.fileno()` returns the saved (pre-redirection) `fd`, so
+{mod}`faulthandler` and similar consumers keep working. Writes to `fd 1` and
+`fd 2` land in the capture tmpfile, so `os.dup2()` calls inside the CLI no
+longer leak into the host runner's stdout.
+
+```{note}
+`capture="fd"` is not available on Windows.
+```
+
+```{versionchanged} 8.4.0
+Added the `capture` parameter. The default `sys` mode no longer exposes the
+original `fd` through `fileno()`, reverting the change introduced in `8.3.3`
+that broke Pytest's `fd`-level capture teardown. Use `capture="fd"` to restore
+that behavior with proper isolation.
 ```

src/click/testing.py

@@ -22,6 +22,8 @@
 
     from .core import Command
 
+CaptureMode = t.Literal["sys", "fd"]
+
 
 class EchoingStdin:
     def __init__(self, input: t.BinaryIO, output: t.BinaryIO) -> None:
@@ -67,6 +69,39 @@ def _pause_echo(stream: EchoingStdin | None) -> cabc.Iterator[None]:
         stream._paused = False
 
 
+class _FDCapture:
+    """Redirect a file descriptor to a temporary file for capture.
+
+    Saves the current target of *targetfd* via :func:`os.dup`, then
+    redirects it to a temporary file via :func:`os.dup2`. On
+    :meth:`stop`, restores the original ``fd`` and returns the captured
+    bytes. Inspired by Pytest's ``FDCapture``.
+
+    .. versionadded:: 8.4.0
+    """
+
+    def __init__(self, targetfd: int) -> None:
+        self._targetfd = targetfd
+        self.saved_fd: int = -1
+        self._tmpfile: t.BinaryIO | None = None
+
+    def start(self) -> None:
+        self.saved_fd = os.dup(self._targetfd)
+        self._tmpfile = tempfile.TemporaryFile(buffering=0)
+        os.dup2(self._tmpfile.fileno(), self._targetfd)
+
+    def stop(self) -> bytes:
+        assert self._tmpfile is not None, "_FDCapture.start() was not called"
+        os.dup2(self.saved_fd, self._targetfd)
+        os.close(self.saved_fd)
+        self.saved_fd = -1
+        self._tmpfile.seek(0)
+        data = self._tmpfile.read()
+        self._tmpfile.close()
+        self._tmpfile = None
+        return data
+
+
 class BytesIOCopy(io.BytesIO):
     """Patch ``io.BytesIO`` to let the written stream be copied to another.
 
@@ -104,29 +139,25 @@ class _NamedTextIOWrapper(io.TextIOWrapper):
     """A :class:`~io.TextIOWrapper` with custom ``name`` and ``mode``
     that does not close its underlying buffer.
 
-    An optional ``original_fd`` preserves the file descriptor of the
-    stream being replaced, so that C-level consumers that call
-    :meth:`fileno` (``faulthandler``, ``subprocess``, ...) still work.
-    Inspired by pytest's ``capsys``/``capfd`` split: see :doc:`/testing`
-    for details.
-
-    .. versionchanged:: 8.3.3
-        Added ``original_fd`` parameter and :meth:`fileno` override.
+    When ``CliRunner`` runs in ``fd`` mode, ``_original_fd`` is patched to
+    point at the saved (pre-redirection) ``fd``, so C-level consumers that call
+    :meth:`fileno` (like ``faulthandler`` or ``subprocess``) keep working. In
+    the default ``sys`` mode ``_original_fd`` stays at ``-1`` and
+    :meth:`fileno` raises :exc:`io.UnsupportedOperation`, matching the
+    pre-``8.3.3`` behavior.
     """
 
     def __init__(
         self,
         buffer: t.BinaryIO,
         name: str,
         mode: str,
-        *,
-        original_fd: int = -1,
         **kwargs: t.Any,
     ) -> None:
         super().__init__(buffer, **kwargs)
         self._name = name
         self._mode = mode
-        self._original_fd = original_fd
+        self._original_fd: int = -1
 
     def close(self) -> None:
         """The buffer this object contains belongs to some other object,
@@ -137,15 +168,10 @@ def close(self) -> None:
         """
 
     def fileno(self) -> int:
-        """Return the file descriptor of the original stream, if one was
-        provided at construction time.
-
-        This allows C-level consumers (``faulthandler``, ``subprocess``,
-        signal handlers, ...) to obtain a valid fd without crashing, even
-        though the Python-level writes are redirected to an in-memory
-        buffer.
-
-        .. versionadded:: 8.3.3
+        """Return the file descriptor of the saved original stream when
+        ``CliRunner`` runs in ``fd`` mode. Otherwise delegate to
+        :class:`~io.TextIOWrapper`, which raises
+        :exc:`io.UnsupportedOperation` for a ``BytesIO``-backed buffer.
         """
         if self._original_fd >= 0:
             return self._original_fd
@@ -272,6 +298,21 @@ class CliRunner:
                        will automatically echo the input.
     :param catch_exceptions: Whether to catch any exceptions other than
                              ``SystemExit`` when running :meth:`~CliRunner.invoke`.
+    :param capture: Selects the output capture strategy. ``sys`` (default)
+        captures Python-level writes only and leaves
+        :meth:`sys.stdout.fileno` raising :exc:`io.UnsupportedOperation`, so
+        user code that calls :func:`os.dup2` on ``sys.stdout.fileno()`` cannot
+        clobber the host runner's stdout. ``fd`` redirects file descriptors
+        ``1`` and ``2`` via :func:`os.dup2` to a temporary file, also catching
+        output from stale stream references, C extensions, and subprocesses.
+        ``fd`` is not supported on Windows.
+
+    .. versionchanged:: 8.4.0
+        Added the ``capture`` parameter. The default ``sys`` mode no longer
+        exposes the original fd through :meth:`fileno`, reverting the change
+        introduced in ``8.3.3`` that broke Pytest's ``fd``-level capture
+        teardown. Use ``capture="fd"`` to restore that behavior with proper
+        isolation. :issue:`3384`
 
     .. versionchanged:: 8.2
         Added the ``catch_exceptions`` parameter.
@@ -286,11 +327,21 @@ def __init__(
         env: cabc.Mapping[str, str | None] | None = None,
         echo_stdin: bool = False,
         catch_exceptions: bool = True,
+        capture: CaptureMode = "sys",
     ) -> None:
+        if capture not in {"sys", "fd"}:
+            raise ValueError(
+                f"capture={capture!r} is not valid. Choose from 'sys' or 'fd'."
+            )
+        if capture == "fd" and sys.platform == "win32":
+            raise ValueError(
+                f"capture={capture!r} is not supported on Windows. Use 'sys'."
+            )
         self.charset = charset
         self.env: cabc.Mapping[str, str | None] = env or {}
         self.echo_stdin = echo_stdin
         self.catch_exceptions = catch_exceptions
+        self.capture: CaptureMode = capture
 
     def get_default_prog_name(self, cli: Command) -> str:
         """Given a command object it will return the default program name
@@ -355,20 +406,6 @@ def isolation(
 
         stream_mixer = StreamMixer()
 
-        # Preserve the original file descriptors so that C-level
-        # consumers (faulthandler, subprocess, etc.) can still obtain a
-        # valid fd from the redirected streams. The original streams
-        # may themselves lack a fileno() (e.g. when CliRunner is used
-        # inside pytest's capsys), so we fall back to -1.
-        def _safe_fileno(stream: t.IO[t.Any]) -> int:
-            try:
-                return stream.fileno()
-            except (AttributeError, io.UnsupportedOperation):
-                return -1
-
-        old_stdout_fd = _safe_fileno(old_stdout)
-        old_stderr_fd = _safe_fileno(old_stderr)
-
         if self.echo_stdin:
             bytes_input = echo_input = t.cast(
                 t.BinaryIO, EchoingStdin(bytes_input, stream_mixer.stdout)
@@ -388,7 +425,6 @@ def _safe_fileno(stream: t.IO[t.Any]) -> int:
             encoding=self.charset,
             name="<stdout>",
             mode="w",
-            original_fd=old_stdout_fd,
         )
 
         sys.stderr = _NamedTextIOWrapper(
@@ -397,7 +433,6 @@ def _safe_fileno(stream: t.IO[t.Any]) -> int:
             name="<stderr>",
             mode="w",
             errors="backslashreplace",
-            original_fd=old_stderr_fd,
         )
 
         @_pause_echo(echo_input)  # type: ignore
@@ -579,7 +614,27 @@ def invoke(
         if catch_exceptions is None:
             catch_exceptions = self.catch_exceptions
 
+        # Set up fd capture before isolation replaces sys.stdout and sys.stderr.
+        cap_out: _FDCapture | None = None
+        cap_err: _FDCapture | None = None
+
+        if self.capture == "fd":
+            cap_out = _FDCapture(1)
+            cap_err = _FDCapture(2)
+            try:
+                cap_out.start()
+                cap_err.start()
+            except OSError:
+                cap_out = cap_err = None
+
         with self.isolation(input=input, env=env, color=color) as outstreams:
+            # Point the captured streams' fileno() at the saved (original)
+            # fd so that C-level consumers like faulthandler keep working
+            # while fd 1/2 are redirected to the capture tmpfile.
+            if cap_out is not None and cap_err is not None:
+                sys.stdout._original_fd = cap_out.saved_fd  # type: ignore[union-attr]
+                sys.stderr._original_fd = cap_err.saved_fd  # type: ignore[union-attr]
+
             return_value = None
             exception: BaseException | None = None
             exit_code = 0
@@ -620,6 +675,18 @@ def invoke(
             finally:
                 sys.stdout.flush()
                 sys.stderr.flush()
+
+                # Stop fd capture and merge the captured bytes into
+                # the stdout/stderr BytesIO streams. BytesIOCopy mirrors
+                # those writes into outstreams[2] automatically.
+                if cap_out is not None and cap_err is not None:
+                    fd_out = cap_out.stop()
+                    fd_err = cap_err.stop()
+                    if fd_out:
+                        outstreams[0].write(fd_out)
+                    if fd_err:
+                        outstreams[1].write(fd_err)
+
                 stdout = outstreams[0].getvalue()
                 stderr = outstreams[1].getvalue()
                 output = outstreams[2].getvalue()