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.

Sign up for GitHub

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

Jump to bottom

echo_via_pager with generators leaves terminal in broken state #2674

Open
0xDEC0DE opened this issue Feb 12, 2024 · 3 comments · May be fixed by #2775
Open

echo_via_pager with generators leaves terminal in broken state #2674

0xDEC0DE opened this issue Feb 12, 2024 · 3 comments · May be fixed by #2775

Comments

@0xDEC0DE
Copy link
Contributor

0xDEC0DE commented Feb 12, 2024 •
edited
Loading

Steps to reproduce

Run the following test case:

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()

Attempt to use the terminal afterwards

Expected result

A working terminal.

Actual behavior

Newlines and output are obscured. Commands may be entered, but they are not displayed.

Workaround

Run the reset command after the command terminates.

Environment:

Reproducible on OS X and Ubuntu (x86_64):

$ sw_vers
ProductName:            macOS
ProductVersion:         14.3.1
BuildVersion:           23D60
  • Python version: 3.11.2
  • Click version: 8.1.7
# cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"
  • Python version: 3.10.12
  • Click version: 8.1.7
@0xDEC0DE
Copy link
Contributor Author

Addendum

If the generator exits "normally", the terminal works fine. It's only when doing things like error handling that it goes sideways.

@stefreak
Copy link
Contributor

stefreak commented Sep 7, 2024

Successfully reproduced this problem using bash, but failed to reproduce using zsh in my case (also macos)

stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
39f36b8 
…cho in the generator

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

When running a program like

```
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.
stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
b62bc06 
…cho in the generator

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

When running a program like

```
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.
@stefreak stefreak linked a pull request Sep 7, 2024 that will close this issue
Open
4 tasks
@stefreak
Copy link
Contributor

stefreak commented Sep 7, 2024 •
edited
Loading

Hi @0xDEC0DE, thank you for the detailed bug report and the repro! I found a solution for the problem in #2775, if this is still relevant to you and you have time for it I'd be happy to receive feedback and/or a quick review 👍

stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
2d07605 
…cho in the generator

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

When running a program like

```
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.
stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
b715ba2 
…cho in the generator

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

When running a program like

```
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.
stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
ce88a00 
…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.
stefreak added a commit to stefreak/click that referenced this issue Sep 7, 2024
@stefreak
Improve echo_via_pager behaviour in face of errors and use of click.e…
8be3685 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve echo_via_pager behaviour in face of errors stefreak/click
2 participants
@stefreak @0xDEC0DE