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!

[geofft]

This is a bit of a driveby comment, sorry, but TIOCSTI is indeed going to be blocked by most distros. The upstream kernel has a strong warning against it in drivers/tty/Kconfig:

config LEGACY_TIOCSTI
        bool "Allow legacy TIOCSTI usage"
        default y
        help
          Historically the kernel has allowed TIOCSTI, which will push
          characters into a controlling TTY. This continues to be used
          as a malicious privilege escalation mechanism, and provides no
          meaningful real-world utility any more. Its use is considered
          a dangerous legacy operation, and can be disabled on most
          systems.

          Say Y here only if you have confirmed that your system's
          userspace depends on this functionality to continue operating
          normally.

          Processes which run with CAP_SYS_ADMIN, such as BRLTTY, can
          use TIOCSTI even when this is set to N.

          This functionality can be changed at runtime with the
          dev.tty.legacy_tiocsti sysctl. This configuration option sets
          the default value of the sysctl.

Also, at least on my machine, a newline after Ctrl-C doesn't actually interrupt, it sends the current line for execution just as if I had not hit Ctrl-C.

I think this is actually going to be hard to get right. The old REPL using Modules/readline.c wants to get interrupted with EINTR, but it also wants to specifically see PyErr_CheckSignals() return nonzero, which it simply will not do not on the main thread. The new REPL also does a little bit too much I/O on its own.

One option is to actually do the blocking I/O on the main thread. I know that's distasteful, but as I understand it, the REPL doesn't give you a nursery, so the only things that would be blocked are system tasks, right?

One option is to do the blocking I/O on the main thread, and take advantage of the fact that both REPLs call some hooks every 100 ms and call into Trio's guest mode. I think you can do this in pure Python for the new REPL; for the old REPL I think you can abuse ctypes against the running Python interpreter to set PyOS_InputHook but it's a little weird.

One option might be to try to get Trio to run on a secondary thread?

The nicest option would be to handle console I/O in Trio itself. For the old REPL you want to use the readline "alternate interface" (rl_callback_handler_install and rl_callback_read_char), which Modules/readline.c uses, but not quite flexibly enough. Follow the logic in that file, and call those two C functions via ctypes. For the new REPL there's a small chunk of code to copy/paste and sprinkle some awaits on, but at least it's pure Python.

[A5rocks]

One option is to actually do the blocking I/O on the main thread.

I think we discussed it a bit and this fails because someone may run this in their REPL:

while True:
  pass

and we want to interrupt that.

For the old REPL you want to use the readline "alternate interface" (rl_callback_handler_install and rl_callback_read_char), which Modules/readline.c uses

Huh does that work with libeditline or whatever the MacOS thing is?

---

Ultimately I think this is probably the best compromise for now, and then we rely on vendored pyrepls which have reimplemented readline in Python which means any sort of changes is easy. (this is what the asyncio REPL relies on)

[geofft]

#3306 is an extremely messy attempt at doing this for the old REPL via calling readline ourselves.

Huh does that work with libeditline or whatever the MacOS thing is?

The code in Modules/readline.c in the Python source seems to use them unconditionally, and the symbols appear to exist in editline, so I think so.

(But the more annoying thing is that this general approach doesn't work with python-build-standalone's builds i.e. what you get from uv and a few other tools. While we're using libedit, that's not the problem here; the problem is we're statically linking it and don't re-export those symbols. That is... something I'm going to have to think about, we have a similar problem with people who want to get at the underlying Tcl/Tk symbols from tkinter.)

[geofft]

Re while True: pass, I think that would work fine with what I'm thinking - the blocking I/O is only when reading input, i.e., InteractiveConsole.raw_input(). Once that's over, you're back in normal async code and Ctrl-C works the way it normally does.

To try it out, change line = await self.async_input(prompt) in #3306 back to line = self.raw_input(prompt), but keep the rest of the changes to run everything on the main thread. Seems to work for me. (If you want to go this route, you can ditch all the weird ctypes stuff in that PR, the relevant change is just that you want async def ainteract() instead of calling the existing interact() in a thread.

[richardsheridan]

I know that's distasteful, but as I understand it, the REPL doesn't give you a nursery, so the only things that would be blocked are system tasks, right?

The system nursery is available and may be expected to function for dispatching background tasks for both the interactive repl user and any number of libraries, so I think this is a nonstarter. unless maybe you do it by polling?