Improve REPL KI

[richardsheridan]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 86.77686% with 16 lines in your changes missing coverage. Please review.

Project coverage is 99.91749%. Comparing base (4f54774) to head (7f135cc).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/trio/_repl.py	65.21739%	15 Missing and 1 partial ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##                 main       #3030         +/-   ##
====================================================
- Coverage   100.00000%   99.91749%   -0.08251%     
====================================================
  Files             127         127                 
  Lines           19265       19392        +127     
  Branches         1302        1324         +22     
====================================================
+ Hits            19265       19376        +111     
- Misses              0          15         +15     
- Partials            0           1          +1     

Files with missing lines	Coverage Δ
src/trio/_tests/test_repl.py	100.00000% <100.00000%> (ø)
src/trio/_repl.py	81.39535% <65.21739%> (-18.60466%)	⬇️

... and 7 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]

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]

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.

[richardsheridan]

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.

[graingert]

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)

[richardsheridan]

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]

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

[A5rocks]

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

I was talking about just sending SIGTERM to a process running the REPL as a test that, well, the REPL supports ctrl+c. This makes it sound like this PR makes us depend on having a terminal attached (and maybe even behavior of that terminal) which sounds not great. If so, then there's probably no way other than maybe using tmux as a parent for the REPL and seeing if that does this special behavior?

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

Interesting, I thought it was copied over from PyPy. Anyways, the interrupt thing is https://github.com/python/cpython/blob/2fd09b011031f3c00c342b44e02e2817010e507c/Lib/asyncio/__main__.py#L135-L142 I think. And then later they have a loop where they try loop.run_forever() and call this interrupt function on KI.

[A5rocks]

I would be happy to push this to the finish line. Here's a couple things I noticed:

1. the extra newline after pressing ctrl+c after a prompt irritates me more than it should. After spending more time than is reasonable, I think the best approach is making interrupted be stored on the class and overwriting write.
2. I started to work on the test case I was thinking about. It seems to imply we need the user to enter a newline or that the ioctl thing only works for shells (?). (I don't need to enter a newline when testing using my terminal...):

import trio
import sys
import subprocess
import signal
from functools import partial

async def main():
    async with trio.open_nursery() as nursery:
        proc = await nursery.start(partial(
            trio.run_process,
            [sys.executable, "-m", "trio"],
            # shell=True,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
            stdin=subprocess.PIPE,
        ))

        async with proc.stdout, proc.stderr:
            # setup
            await anext(proc.stderr)
            assert await anext(proc.stdout) == b'>>> '

            # ensure things work
            await proc.stdin.send_all(b'print("hello!")\n')
            assert await anext(proc.stdout) == b'hello!\n>>> '

            # ensure that ctrl+c on the REPL works
            proc.send_signal(signal.SIGINT)
            print("stdout:")
            with trio.move_on_after(0.5):
                async for c in proc.stdout:
                    print(c)

            print("stderr:")
            with trio.move_on_after(0.5):
                async for c in proc.stderr:
                    print(c)

            await proc.stdin.send_all(b'\n')
            print("stdout:")
            with trio.move_on_after(0.5):
                async for c in proc.stdout:
                    print(c)

            print("stderr:")
            with trio.move_on_after(0.5):
                async for c in proc.stderr:
                    print(c)

        # kill the process
        nursery.cancel_scope.cancel()

trio.run(main)

output:

stdout:
stderr:
stdout:
b'>>> '
stderr:
b'KeyboardInterrupt\n'

Generally this looks great though.

[richardsheridan]

Thanks for taking interest in completing this!

1. the extra newline after pressing ctrl+c after a prompt irritates me more than it should. After spending more time than is reasonable, I think the best approach is making interrupted be stored on the class and overwriting write.

This is an effort/reward balance that wouldn't quite make the cut for me, but I won't try to stop you!

2. I started to work on the test case I was thinking about. It seems to imply we need the user to enter a newline or that the ioctl thing only works for shells (?). (I don't need to enter a newline when testing using my terminal...):

I thought the ioctl somehow uses a kernel api to send bytes through the process's attached terminal so i'm not surprised that it only works in some cases, but I also don't know exactly what those cases would be without trying. What happened to your tmux idea?

[A5rocks]

What happened to your tmux idea?

Unfortunately that doesn't seem to help -- I've tried both shell=True (which implies sh -c "...") and explicitly using tmux -c "...", but the ioctl call doesn't seem to be inserting a newline. I suppose it's fine to not be able to test that, but that's really not ideal...

[A5rocks]

Ok it seems this relies on having a PTY, which is something we can provide it. Maybe one day CPython will export pyrepl so we can do what asyncio does and take advantage of the fact they use a reimplemented readline...

[A5rocks]

Turns out the reason my test code above wasn't working is somewhat trivial: terminal_newline is suppressing OSError!

So far I get:

• using subprocess.PIPE for stdin, etc gives OSError 25 "Inappropriate ioctl for device" (makes sense...)
• using a PTY yields OSError 1 "Operation not permitted"

The latter seems closer, but I'm not quite getting Linux's permission model 🤔

[A5rocks]

It's actually impossible to test this in CI... because it doesn't work in CI! Linux 6.2 introduced a sysctl to make the TIOCSTI ioctl require superuser (CAP_SYS_ADMIN), and guess what Ubuntu 24.04 enabled :/ (I didn't notice because my Debian system is on Linux 6.1...)

You can check your own system with sysctl dev.tty.legacy_tiocsti. I guess we can just say the implementation is best-effort and call it a day? We can keep the test that ctrl+c works elsewhere + inject a newline in the test case itself for non-Windows. This is still an improvement over status quo I suppose...

Please review this!

EDIT: I really dislike how... not great the tests are. I think we should rely on vendored pyrepls given we support only CPython/PyPy, especially since the functionality we need is basically at a "rewrite readline" level. Though any changes should be on another PR.

[richardsheridan]

It's actually impossible to test this in CI... because it doesn't work in CI! Linux 6.2 introduced a sysctl to make the TIOCSTI ioctl require superuser (CAP_SYS_ADMIN), and guess what Ubuntu 24.04 enabled :/ (I didn't notice because my Debian system is on Linux 6.1...)

I'll try to review this weekend, but just quickly wanted to ask if this means that eventually the ioctl will break on (essentially) all linux users?

[A5rocks]

Only if the distro disables the ioctl, so maybe not all? But yeah we will need to brainstorm another solution...

[richardsheridan]

I suppose the tests make sense to me overall, it's just unfortunate that there are still some holes in them. If it is just a best-effort holdover until a pyrepl based solution can be stitched together then I don't think that should be a blocker. It's still better than the present behavior for all users.

[richardsheridan]

Instead of simply suppressing OSError, maybe catch it and prompt the user to press enter to continue by printing text? We could also get the OSError code at that time which could be useful for understanding the situation.

[A5rocks]

Hopefully my change worked, I'm not on a Linux machine so I cannot check yet.

[richardsheridan]

I never dug in to why EOFError pops out, maybe that knowledge could help us test here

[A5rocks]

I haven't dug into this yet unfortunately.

[A5rocks]

I want to do this but still haven't:

• make the test skip only if the relevant sysctl is enabled
• add a bunch of codecov skips....

Sorry about the delay, I've been on vacation!