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

gh-120221: Support KeyboardInterrupt in asyncio REPL #123795

Merged
merged 4 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions Lib/_pyrepl/_threading_handler.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
from __future__ import annotations

from dataclasses import dataclass, field
import traceback


TYPE_CHECKING = False
if TYPE_CHECKING:
from threading import Thread
from types import TracebackType
from typing import Protocol

class ExceptHookArgs(Protocol):
@property
def exc_type(self) -> type[BaseException]: ...
@property
def exc_value(self) -> BaseException | None: ...
@property
def exc_traceback(self) -> TracebackType | None: ...
@property
def thread(self) -> Thread | None: ...

class ShowExceptions(Protocol):
def __call__(self) -> int: ...
def add(self, s: str) -> None: ...

from .reader import Reader


def install_threading_hook(reader: Reader) -> None:
import threading
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this import here so that pyrepl will continue to work in thread-free environments like iOS and the browser.

Copy link
Member

Choose a reason for hiding this comment

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

FYI: iOS and Android do support threads, but not subprocesses. The only platform that doesn't support threads is WASM.


@dataclass
class ExceptHookHandler:
lock: threading.Lock = field(default_factory=threading.Lock)
messages: list[str] = field(default_factory=list)

def show(self) -> int:
count = 0
with self.lock:
if not self.messages:
return 0
reader.restore()
for tb in self.messages:
count += 1
if tb:
print(tb)
self.messages.clear()
reader.scheduled_commands.append("ctrl-c")
reader.prepare()
return count

def add(self, s: str) -> None:
with self.lock:
self.messages.append(s)

def exception(self, args: ExceptHookArgs) -> None:
lines = traceback.format_exception(
args.exc_type,
args.exc_value,
args.exc_traceback,
colorize=reader.can_colorize,
) # type: ignore[call-overload]
pre = f"\nException in {args.thread.name}:\n" if args.thread else "\n"
tb = pre + "".join(lines)
self.add(tb)

def __call__(self) -> int:
return self.show()


handler = ExceptHookHandler()
reader.threading_hook = handler
threading.excepthook = handler.exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This except hook essentially queues tracebacks for nicer display whenever the REPL is actually ready to do it.

42 changes: 28 additions & 14 deletions Lib/_pyrepl/reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@

# types
Command = commands.Command
if False:
from .types import Callback, SimpleContextManager, KeySpec, CommandName
from .types import Callback, SimpleContextManager, KeySpec, CommandName


def disp_str(buffer: str) -> tuple[str, list[int]]:
Expand Down Expand Up @@ -247,6 +246,7 @@ class Reader:
lxy: tuple[int, int] = field(init=False)
scheduled_commands: list[str] = field(default_factory=list)
can_colorize: bool = False
threading_hook: Callback | None = None

## cached metadata to speed up screen refreshes
@dataclass
Expand Down Expand Up @@ -722,6 +722,24 @@ def do_cmd(self, cmd: tuple[str, list[str]]) -> None:
self.console.finish()
self.finish()

def run_hooks(self) -> None:
threading_hook = self.threading_hook
if threading_hook is None and 'threading' in sys.modules:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This automatic installation is such that when people create threads in the REPL directly or indirectly (via libraries), they get correctly formatted tracebacks.

It just so happens that asyncio REPL uses threads so it triggers this condition, too.

from ._threading_handler import install_threading_hook
install_threading_hook(self)
if threading_hook is not None:
try:
threading_hook()
except Exception:
pass

input_hook = self.console.input_hook
if input_hook:
try:
input_hook()
except Exception:
pass

def handle1(self, block: bool = True) -> bool:
"""Handle a single event. Wait as long as it takes if block
is true (the default), otherwise return False if no event is
Expand All @@ -732,16 +750,13 @@ def handle1(self, block: bool = True) -> bool:
self.dirty = True

while True:
input_hook = self.console.input_hook
if input_hook:
input_hook()
# We use the same timeout as in readline.c: 100ms
while not self.console.wait(100):
input_hook()
event = self.console.get_event(block=False)
else:
event = self.console.get_event(block)
if not event: # can only happen if we're not blocking
# We use the same timeout as in readline.c: 100ms
self.run_hooks()
self.console.wait(100)
event = self.console.get_event(block=False)
if not event:
if block:
continue
return False

translate = True
Expand All @@ -763,8 +778,7 @@ def handle1(self, block: bool = True) -> bool:
if cmd is None:
if block:
continue
else:
return False
return False

self.do_cmd(cmd)
return True
Expand Down
15 changes: 13 additions & 2 deletions Lib/_pyrepl/unix_console.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,14 @@ def _my_getstr(cap: str, optional: bool = False) -> bytes | None:
self.event_queue = EventQueue(self.input_fd, self.encoding)
self.cursor_visible = 1

def more_in_buffer(self) -> bool:
return bool(
self.input_buffer
and self.input_buffer_pos < len(self.input_buffer)
)

def __read(self, n: int) -> bytes:
if not self.input_buffer or self.input_buffer_pos >= len(self.input_buffer):
if not self.more_in_buffer():
self.input_buffer = os.read(self.input_fd, 10000)

ret = self.input_buffer[self.input_buffer_pos : self.input_buffer_pos + n]
Expand Down Expand Up @@ -393,6 +399,7 @@ def get_event(self, block: bool = True) -> Event | None:
"""
if not block and not self.wait(timeout=0):
return None

while self.event_queue.empty():
while True:
try:
Expand All @@ -413,7 +420,11 @@ def wait(self, timeout: float | None = None) -> bool:
"""
Wait for events on the console.
"""
return bool(self.pollob.poll(timeout))
return (
not self.event_queue.empty()
or self.more_in_buffer()
or bool(self.pollob.poll(timeout))
)
Comment on lines +423 to +427
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous behavior caused pyrepl to lose reads in non-blocking mode (i.e. with an input hook) if more than 1 character was read previously to the input_buffer (only 1 character is turned into an event at a time). This made tests flaky. Now they pass every time.


def set_cursor_vis(self, visible):
"""
Expand Down
2 changes: 1 addition & 1 deletion Lib/_pyrepl/windows_console.py
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ def wait(self, timeout: float | None) -> bool:
while True:
if msvcrt.kbhit(): # type: ignore[attr-defined]
return True
if timeout and time.time() - start_time > timeout:
if timeout and time.time() - start_time > timeout / 1000:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha

Copy link
Member

Choose a reason for hiding this comment

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

lol

return False
time.sleep(0.01)

Expand Down
10 changes: 10 additions & 0 deletions Lib/asyncio/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ def run(self):

loop.call_soon_threadsafe(loop.stop)

def interrupt(self) -> None:
if not CAN_USE_PYREPL:
return

from _pyrepl.simple_interact import _get_reader
r = _get_reader()
if r.threading_hook is not None:
r.threading_hook.add("") # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we add a KeyboardInterrupt to the other thread. We don't want a traceback, and the "KeyboardInterrupt" message will already appear.



if __name__ == '__main__':
sys.audit("cpython.run_stdin")
Expand Down Expand Up @@ -184,6 +193,7 @@ def run(self):
keyboard_interrupted = True
if repl_future and not repl_future.done():
repl_future.cancel()
repl_thread.interrupt()
continue
else:
break
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_pyrepl/support.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ def flushoutput(self) -> None:
def forgetinput(self) -> None:
pass

def wait(self) -> None:
pass
def wait(self, timeout: float | None = None) -> bool:
return True

def repaint(self) -> None:
pass
5 changes: 3 additions & 2 deletions Lib/test/test_repl.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,15 @@ def test_asyncio_repl_reaches_python_startup_script(self):
def test_asyncio_repl_is_ok(self):
m, s = pty.openpty()
cmd = [sys.executable, "-I", "-m", "asyncio"]
env = os.environ.copy()
proc = subprocess.Popen(
cmd,
stdin=s,
stdout=s,
stderr=s,
text=True,
close_fds=True,
env=os.environ,
env=env,
)
os.close(s)
os.write(m, b"await asyncio.sleep(0)\n")
Expand All @@ -270,7 +271,7 @@ def test_asyncio_repl_is_ok(self):
proc.kill()
exit_code = proc.wait()

self.assertEqual(exit_code, 0)
self.assertEqual(exit_code, 0, "".join(output))

class TestInteractiveModeSyntaxErrors(unittest.TestCase):

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
asyncio REPL is now again properly recognizing KeyboardInterrupts. Display
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
asyncio REPL is now again properly recognizing KeyboardInterrupts. Display
asyncio REPL is now again properly recognizing :exc:`KeyboardInterrupt`. Display

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the same word. Let's do stuff like that in a subsequent PR please, so we don't waste time on CI now.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The upcoming release is more important now.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR, to put the s, you would do :exc:`KeyboardInterrupt`\s

of exceptions raised in secondary threads is fixed.
Loading