Skip to content

fix I/O operation on closed file error with CliRunner and echo_via_pager#3482

Merged
davidism merged 1 commit into
pallets:stablefrom
ChrisPappalardo:fix-io-operation-on-closed-file-error
May 22, 2026
Merged

fix I/O operation on closed file error with CliRunner and echo_via_pager#3482
davidism merged 1 commit into
pallets:stablefrom
ChrisPappalardo:fix-io-operation-on-closed-file-error

Conversation

@ChrisPappalardo
Copy link
Copy Markdown
Contributor

Fixes #3449 by adding a thinner version of MaybeStripAnsi that is not subclassed from io.TextIOWrapper when handling sys.stdout and sys.stderr streams.

@davidism
Copy link
Copy Markdown
Member

Seems like we should be able to keep the single wrapper and override the close method to do nothing in the problematic case.

@davidism davidism added this to the 8.4.1 milestone May 21, 2026
Comment thread src/click/_termui_impl.py Outdated
# while text-only streams are yielded as-is.
if _has_binary_buffer(stream):
# route stdout and stderr through a thinner wrapper
is_std = stream is sys.stdout or stream is sys.stderr
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rowlando13 this is a good example of why I think the whole text/binary stream handling in Click is not needed. It's completely bypassed for stdin/stdout here, and all tests still pass. Fairly sure these checks were for Python 2/3 compat, but I didn't have enough time to check it all when I removed most of the compat years ago.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being discussed at #3481

@davidism
Copy link
Copy Markdown
Member

After investigating from my initial comment, it does look like we need to wrap the stream returned by nullpager instead of this. The wrapper should pass everything through, except override close to do nothing. Will work on this tonight or tomorrow.

@davidism davidism force-pushed the fix-io-operation-on-closed-file-error branch from 2cf8e01 to becbde5 Compare May 22, 2026 03:36
@davidism davidism changed the base branch from main to stable May 22, 2026 03:37
@davidism
Copy link
Copy Markdown
Member

Also needed to make sure stream.buffer wouldn't close either, since get_pager_file would wrap stream.buffer instead of stream. This all seems a bit messy, probably something that can be cleaned up eventually. @Rowlando13 @kdeldycke.

@davidism davidism merged commit 5ee8e31 into pallets:stable May 22, 2026
12 checks passed
@kdeldycke kdeldycke added bug f:test runner feature: cli test runner labels May 22, 2026
@kdeldycke
Copy link
Copy Markdown
Collaborator

Also needed to make sure stream.buffer wouldn't close either, since get_pager_file would wrap stream.buffer instead of stream. This all seems a bit messy, probably something that can be cleaned up eventually. @Rowlando13 @kdeldycke.

Also CC @AndreasBackx for the missing tests in #3405 covering #1572

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

Labels

bug f:test runner feature: cli test runner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"I/O operation on closed file" error with CliRunner and echo_via_pager under 8.4

3 participants