From 8b6441f1759b521eb1f09e4daf01c255c3d906b6 Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Fri, 29 May 2026 15:30:33 +0200 Subject: [PATCH] Do not let the GC close a borrowed `stdout` Complete the "I/O operation on closed file" fix from #3482, and definitely addresses #3449. --- CHANGES.rst | 4 +++ src/click/_termui_impl.py | 67 +++++++++++++++++++++++---------------- src/click/utils.py | 12 +++++++ tests/test_termui.py | 56 ++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 27 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 14f575ad5..be712ea98 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -14,6 +14,10 @@ Unreleased - A :class:`Group` with ``invoke_without_command=True`` marks its subcommand as optional in the usage help, showing ``[COMMAND]`` instead of ``COMMAND``. :issue:`3059` :pr:`3507` +- ``echo_via_pager`` and ``get_pager_file`` no longer close a borrowed stdout + stream when no external pager runs, completing the partial + ``I/O operation on closed file`` fix from :pr:`3482`. :issue:`3449` + :pr:`3533` Version 8.4.1 diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index 76113e9bb..b64a17237 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -29,6 +29,7 @@ from ._compat import WIN from .exceptions import ClickException from .utils import echo +from .utils import KeepOpenFile V = t.TypeVar("V") @@ -438,17 +439,28 @@ def get_pager_file(color: bool | None = None) -> t.Generator[t.TextIO, None, Non with _pager_contextmanager(color=color) as (stream, encoding, color): # Split streams by capabilities rather than the abstract TextIO / # BinaryIO annotations: buffered text streams can be unwrapped to bytes, - # while text-only streams are yielded as-is. + # while other streams are yielded as-is. + wrapper: MaybeStripAnsi | None = None if _has_binary_buffer(stream): # Text stream backed by a binary buffer. - stream = MaybeStripAnsi(stream.buffer, color=color, encoding=encoding) - elif isinstance(stream, t.BinaryIO): - # Binary stream - stream = MaybeStripAnsi(stream, color=color, encoding=encoding) + wrapper = MaybeStripAnsi(stream.buffer, color=color, encoding=encoding) + stream = wrapper try: - yield stream + # Narrow the BinaryIO | TextIO union that _pager_contextmanager + # yields; the caller writes text to the pager. + yield t.cast(t.TextIO, stream) finally: - stream.flush() + try: + stream.flush() + finally: + # Hand the binary buffer back to the pager that produced it + # rather than letting this TextIOWrapper close it on garbage + # collection. The pager owns the buffer's lifecycle: subprocess + # pipes and temp files are closed by their own helpers, while a + # borrowed stdout must stay open for the caller. detach() runs + # even if flush() raised, so the buffer is never closed here. + if wrapper is not None: + wrapper.detach() @contextlib.contextmanager @@ -468,8 +480,11 @@ def _pipepager( """ # Split the command into the invoked CLI and its parameters. if not cmd_parts: + # No usable pager: fall back to stdout through _nullpager so it gets the + # same borrowed-stream handling and the caller's stream is not closed. stdout = _default_text_stdout() or StringIO() - yield stdout, "utf-8", False + with _nullpager(stdout, color) as rv: + yield rv return import shutil @@ -479,8 +494,11 @@ def _pipepager( cmd_filepath = shutil.which(cmd) if not cmd_filepath: + # No usable pager: fall back to stdout through _nullpager so it gets the + # same borrowed-stream handling and the caller's stream is not closed. stdout = _default_text_stdout() or StringIO() - yield stdout, "utf-8", False + with _nullpager(stdout, color) as rv: + yield rv return # Produces a normalized absolute path string. @@ -568,8 +586,11 @@ def _tempfilepager( """ # Split the command into the invoked CLI and its parameters. if not cmd_parts: + # No usable pager: fall back to stdout through _nullpager so it gets the + # same borrowed-stream handling and the caller's stream is not closed. stdout = _default_text_stdout() or StringIO() - yield stdout, "utf-8", False + with _nullpager(stdout, color) as rv: + yield rv return import shutil @@ -579,8 +600,11 @@ def _tempfilepager( cmd_filepath = shutil.which(cmd) if not cmd_filepath: + # No usable pager: fall back to stdout through _nullpager so it gets the + # same borrowed-stream handling and the caller's stream is not closed. stdout = _default_text_stdout() or StringIO() - yield stdout, "utf-8", False + with _nullpager(stdout, color) as rv: + yield rv return # Produces a normalized absolute path string. @@ -606,21 +630,6 @@ def _tempfilepager( os.unlink(f.name) -class _SkipClose: - def __init__(self, stream: t.IO[t.Any]) -> None: - self.stream = stream - - def __getattr__(self, name: str) -> t.Any: - return getattr(self.stream, name) - - @property - def buffer(self) -> t.BinaryIO: - return _SkipClose(self.stream.buffer) # type: ignore[attr-defined, return-value] - - def close(self) -> None: - pass - - @contextlib.contextmanager def _nullpager( stream: t.TextIO, color: bool | None = None @@ -628,13 +637,17 @@ def _nullpager( """Simply print unformatted text. This is the ultimate fallback. Don't close the output stream in this case, since it's coming from elsewhere rather than our internal helpers. + + The stream is wrapped in :class:`~click.utils.KeepOpenFile` so that, as a + borrowed stream, it is not closed by a ``with`` block. The wrapper that + :func:`get_pager_file` builds around it is detached rather than closed. """ encoding = get_best_encoding(stream) if color is None: color = False - yield _SkipClose(stream), encoding, color # type: ignore[misc] + yield KeepOpenFile(stream), encoding, color # type: ignore[misc] class Editor: diff --git a/src/click/utils.py b/src/click/utils.py index 67922b894..c0cb22d68 100644 --- a/src/click/utils.py +++ b/src/click/utils.py @@ -205,6 +205,18 @@ def __iter__(self) -> cabc.Iterator[t.AnyStr]: class KeepOpenFile: + """Proxy a file object but keep it open across a ``with`` block. + + Wraps a borrowed file (such as ``sys.stdin`` or ``sys.stdout``) so that + leaving a ``with`` block does not close it, as used by :func:`open_file` + for the ``-`` filename. The caller stays responsible for the file: an + explicit :meth:`close` still passes through to the wrapped object. + + Dunder methods are proxied explicitly: implicit special-method lookups + bypass :meth:`__getattr__`, because Python resolves them on the type rather + than the instance. + """ + _file: t.IO[t.Any] def __init__(self, file: t.IO[t.Any]) -> None: diff --git a/tests/test_termui.py b/tests/test_termui.py index f715154ee..d4ed5ee19 100644 --- a/tests/test_termui.py +++ b/tests/test_termui.py @@ -1,4 +1,5 @@ import contextlib +import gc import io import platform import shlex @@ -768,6 +769,61 @@ def test_get_pager_file_pager_unset_falls_back_when_no_default(monkeypatch, tmp_ assert pager_out.read_text(encoding="utf-8") == "hello\n" +def test_get_pager_file_missing_pager_keeps_borrowed_stream_open(monkeypatch): + """A missing ``PAGER`` must not close the borrowed stdout (issue #3449). + + The ``8.4.0`` regression was only fixed for the no-tty ``_nullpager`` path; + the ``_pipepager``/``_tempfilepager`` fallbacks (reached in a tty when + ``PAGER`` resolves to nothing) used to close the borrowed stream too. + """ + buffer = io.BytesIO() + stream = io.TextIOWrapper(buffer, encoding="utf-8") + + monkeypatch.setitem( + click._termui_impl.os.environ, + "PAGER", + "click-tests-nonexistent-pager-9b3f2", + ) + monkeypatch.setattr(click._termui_impl, "isatty", lambda _: True) + monkeypatch.setattr(click._termui_impl, "_default_text_stdout", lambda: stream) + + with click.get_pager_file() as pager: + pager.write("hello\n") + + # Drop the wrapper reference and force finalization: the old bug closed the + # borrowed buffer when the TextIOWrapper built by get_pager_file was + # garbage-collected. + del pager + gc.collect() + + assert not buffer.closed + assert not stream.closed + assert buffer.getvalue().replace(b"\r\n", b"\n") == b"hello\n" + + +def test_echo_via_pager_tty_pager_missing(runner, monkeypatch): + """``echo_via_pager`` through the tty fallback keeps ``CliRunner`` working. + + Regression for issue #3449 via the pager fallback: a tty with ``PAGER`` + pointing at a missing binary used to close the runner's stdout, breaking + ``CliRunner.invoke``. + """ + monkeypatch.setattr(click._termui_impl, "isatty", lambda _: True) + monkeypatch.setitem( + click._termui_impl.os.environ, + "PAGER", + "click-tests-nonexistent-pager-9b3f2", + ) + + @click.command() + def cli(): + click.echo_via_pager("Hello, Click!") + + result = runner.invoke(cli) + assert not result.exception + assert result.output == "Hello, Click!\n" + + @pytest.mark.parametrize( ("color", "expected"), [