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 REPL KI #3030

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

richardsheridan
Copy link
Contributor

This commit has a draft method to fix #3007. I have 0 ideas on how to test this thing: not only does it use the raw_input that gets patched for testing, but the way it injects a newline is basically impossible for a test runner to pass in. Also, it feels fragile somehow to dip in and out of the trio loop patching the signal handler and peeking for KeyboardInterrupt.

@richardsheridan richardsheridan marked this pull request as draft July 5, 2024 23:08
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 56.15561%. Comparing base (d63f3a9) to head (240f9ff).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
src/trio/_repl.py 75.00000% 9 Missing ⚠️

❌ Your project check has failed because the head coverage (56.15561%) is below the target coverage (100.00000%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (d63f3a9) and HEAD (240f9ff). Click for more details.

HEAD has 124 uploads less than BASE
Flag BASE (d63f3a9) HEAD (240f9ff)
3.11 13 5
Cython 8 4
3.9 25 9
3.13 13 5
Ubuntu 24 8
macOS 19 7
3.12 15 5
3.10 11 4
3.14 11 4
Alpine 3 1
Windows 43 15
pypy-3.10 9 3
Additional details and impacted files
@@                  Coverage Diff                  @@
##                 main       #3030          +/-   ##
=====================================================
- Coverage   100.00000%   56.15561%   -43.84439%     
=====================================================
  Files             124         246         +122     
  Lines           18764       37608       +18844     
  Branches         1269        2540        +1271     
=====================================================
+ Hits            18764       21119        +2355     
- Misses              0       16361       +16361     
- Partials            0         128         +128     
Files with missing lines Coverage Δ
src/trio/_repl.py 88.31169% <75.00000%> (-11.68832%) ⬇️

... and 132 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@A5rocks
Copy link
Contributor

A5rocks commented Jul 10, 2024

Could we spawn the REPL as another process and send it a signal? (simulate ctrl+c)

I think we could first send it some data over stdin to get into specific scenarios.

@richardsheridan
Copy link
Contributor Author

The newline is sent to the terminal I think, rather than any specific process.

Potentially we could avoid the ioctl weirdness by allowing a small amount of user discomfort in needing to press enter after Ctrl+C.

@A5rocks A5rocks self-requested a review December 7, 2024 00:08
@richardsheridan
Copy link
Contributor Author

richardsheridan commented Feb 14, 2025

Still no idea how to test the proposed fixes. I think it's relevant that asyncio isn't able to interrupt the input thread either, unless it's using the new python repl:

https://github.com/python/cpython/blob/e65e9f90626a4c62da4d3500044f354b51e51dbb/Lib/asyncio/__main__.py#L132-L139

Was there ever talk about the _pyrepl going public? if so, we could use a version of that interrupt method instead.

nonlocal interrupted
interrupted = True
# Fake up a newline char as if user had typed it at terminal
fcntl.ioctl(sys.stdin, termios.TIOCSTI, b"\n")
Copy link
Member

Choose a reason for hiding this comment

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

you can't write to sys.stdin from a handler safely, I'm not sure if you can icotl to the .fileno() or not either, I think you need:

@disable_ki_protection
def _newline():
    # Fake up a newline char as if user had typed it at terminal
    fcntl.ioctl(sys.stdin, termios.TIOCSTI, b"\n")


class TrioInteractiveConsole:
    token = trio.lowlevel.current_trio_token()

    def handler(...):
        nonlocal interrupted
        interrupted = True
        token.run_sync_soon(_newline, idempotent=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think I understand why, but won't a keyboard interrupt in the newline function blow up trio and end the repl? If so, Would suppressing the exception be sufficient?

@richardsheridan
Copy link
Contributor Author

Was there ever talk about the _pyrepl going public? if so, we could use a version of that interrupt method instead.

Uh turns out pyrepl is a separate package that cpython vendored: https://pypi.org/project/pyrepl/#history

I have no intention of vendoring it or making a dependency for this pr but just FYI

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.

Ctrl+C behavior problems at the new REPL
4 participants