Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve echo_via_pager behaviour in face of errors #2775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefreak
Copy link
Contributor

@stefreak stefreak commented Sep 7, 2024

fixes #2674 (echo_via_pager with generators leaves terminal in broken state).

  • Add tests that demonstrate the correct behavior of the change. Tests
    should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.

@stefreak
Copy link
Contributor Author

stefreak commented Sep 7, 2024

I've had a look at #350 and the new behaviour sounds like what @amjith describes in the comment #350 (comment)

The fix by @untitaker is still in place (https://github.com/pallets/click/commits/e71f0a040a81e03276b25ab1d1b0671822ab331b) – now in the finally block, which should be even better.

For me this works really well in all the situations I was able to come up with so far.

@@ -424,26 +424,33 @@ def _pipepager(generator: cabc.Iterable[str], cmd: str, color: bool | None) -> N
text = strip_ansi(text)

stdin.write(text.encode(encoding, "replace"))
except (OSError, KeyboardInterrupt):
Copy link
Contributor Author

@stefreak stefreak Sep 7, 2024

Choose a reason for hiding this comment

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

I'm not sure why OSError has been captured here; I think it's probably better to remove it and re-introduce it with a comment or a more specific solution if it causes a problem.

Copy link
Contributor Author

@stefreak stefreak Sep 7, 2024

Choose a reason for hiding this comment

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

OSError used to be IOError and it's in since the beginning (aef1650).

If write fails for some reason, I'd like to see an error message, rather than swallowing any error that might occur – but I'm happy to revert this change if the reviewer disagrees or there is a reason I'm not aware of

Copy link
Contributor

Choose a reason for hiding this comment

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

most likely this is done for less noisy behavior in broken pipes or broken SSH sessions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, now I also ran into a case. Pushed a version now that ignores BrokenPipeError specifically

src/click/_termui_impl.py Outdated Show resolved Hide resolved
…cho in the generator

This commit fixes pallets#2674 (echo_via_pager with generators leaves terminal in broken state).

When running the program to reproduce the issue (thanks to @0xDEC0DE for providing it in the issue mentioned above!)

```
import click

def output_generator():
    counter = 0
    while True:
        yield "this is a line of output\n"
        if counter == 1024:
            click.echo("kaboom", err=True)
            click.get_current_context().exit(0)
        counter += 1

@click.command
def kaboom():
    click.echo_via_pager(output_generator)

if __name__ == "__main__":
    kaboom()
```

The "kaboom" message will now be visible immediately (the error will cause click to terminate the pager). ZSH and bash will not be in a broken state (tested using bash and zsh on macos).

Further changes:
- Error handling: terminate the pager and close the file descriptors in a finally block
- KeyboardInterrupt: the pager code will not ignore KeyboardInterrupt anymore. This allows the user to
  search for future output of the generator when using less and then aborting the program using ctrl-c.
- tests: improved the echo_via_pager tests by using a named tuple and separate assertions for pager
  output, stderr and stdout. Added additional test cases.
Copy link
Contributor

@0xDEC0DE 0xDEC0DE left a comment

Choose a reason for hiding this comment

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

I don't have rights to approve, but this looks like it will do the trick.

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.

echo_via_pager with generators leaves terminal in broken state
3 participants