Skip to content

馃悰 Keep CliRunner output buffers open when its stream wrappers close - #1977

Open
chrikrah wants to merge 1 commit into
fastapi:masterfrom
chrikrah:fix/clirunner-closed-file
Open

chrikrah wants to merge 1 commit into
fastapi:masterfrom
chrikrah:fix/clirunner-closed-file

Conversation

@chrikrah

Copy link
Copy Markdown

This ports the typer/testing.py half of the Click fix pallets/click#3139, which shipped in Click 8.3.2. Refs #1289, a discussion, so nothing closes on merge.

_NamedTextIOWrapper wraps a StreamMixer buffer. A TextIOWrapper closes its buffer on close() and on garbage collection. pytest's live logging swaps sys.stdout mid-command, the wrapper dies, and invoke() then fails at outstreams[0].getvalue(). The fix has Click's two hunks. The wrapper gets an empty close(), and StreamMixer.__del__ goes, because it closed the buffers again after invoke() returned.

$ python -m pytest --log-cli-level=INFO test_hello.py    # the two files from #1289
# b15210b: E  ValueError: I/O operation on closed file.  (typer/testing.py:329)
# this branch: 1 passed

Each new test in tests/test_termui.py pins one hunk:

$ pytest tests/test_termui.py -k stream_ -q
# b15210b:               2 failed, ValueError: I/O operation on closed file.
# close() only:          test_clirunner_stream_usable_after_invoke fails
# __del__ removed only:  test_clirunner_stream_replaced_during_invoke fails
# this branch:           2 passed
$ bash scripts/test.sh -n 4    # Python 3.10, Linux
# b15210b: 1481 passed, 22 skipped, 2 xfailed. this branch: 1483 passed, 22 skipped, 2 xfailed
# typer/testing.py 100% on both
$ bash scripts/lint.sh    # mypy 2.3.1, ty 0.0.84, ruff 0.16.9
# clean

I did not add a release-notes entry, since latest-changes writes it on merge.

@svlandeg, this is the minimal PR you asked for in #1289. I kept the __del__ removal because the second test fails without it. If you would prefer only the close() hunk here, I'll drop it. I have two more small Click ports built and will hold them until this one is reviewed, unless you'd rather not get them at all.

Port the `typer/testing.py` part of pallets/click#3139, released in Click 8.3.2.
`_NamedTextIOWrapper` gets an empty `close()`, so dropping the wrapper no longer
closes the `StreamMixer` buffer that `invoke()` reads afterwards, and
`StreamMixer.__del__` goes, so a stream captured inside the command stays writable
after `invoke()` returns.
@pjonsson

Copy link
Copy Markdown

I kept the __del__ removal because the second test fails without it. If you would prefer only the close() hunk here, I'll drop it.

The empty close method is to prevent the buffer from being closed by someone who doesn't own it. The del method was added to fix some race condition discovered with freethreaded Python 3.14. My diagnosis was that adding the del method made test case work, but the ownership issue was the root cause for both of them.

I would suggest to get both fixes merged, but it's up to the maintainers with how they want the pull requests organized.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants