bpo-39622: Interrupt the main asyncio task on Ctrl+C

[asvetlov]

The strategy is:

1. If asyncio runner is executed in the main thread it can handle Ctrl+C
2. Ctrl+C cancels the main task (created for executing runner.run(coro())).
3. The second and following Ctrl+C don't cancel the main task but raise KeyboardInterrupt instead. It can break loops, blocking calls etc.
4. runner.run() raises KeyboardInterrupt when the main task is cancelled by Ctrl+C
5. Background tasks are not touched, they are cancelled at the runner finalization stage.

https://bugs.python.org/issue39622

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit 2fd2ab7 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[asvetlov]

@1st1 @gvanrossum the PR adds Ctrl+C handling to asyncio programs.
Please review if you have time.

[kumaraditya303]

LGTM, tested on linux.

[asvetlov]

Thank you very much, @kumaraditya303 !

[gvanrossum]

I'm trying this with code like this:

async def looper():
    while True:
        print("Top...")
        try:
            await asyncio.sleep(1.0)
        except BaseException as err:
            print("Caught", repr(err))

and its behavior is very unpredictable. The first time I hit ^C it will show that it caught CancelledError, maybe the second time too, but after that something weird happens: it shows the KeyboardInterrupt (in runners.py on L142) causing another KeyboardInterrupt (selectors.py L561 on macOS, somewhere in windows_events.py on Windows), then the code prints "Caught GeneratorExit()", and then an endless loop of alternating "Top..." and "Caught RuntimeError('no running event loop')" until I hit another ^C, which stops it.

If I special-case GeneratorExit to break out of the loop, there is no endless loop, but I see something like this (on Windows):

Traceback (most recent call last):
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\runners.py", line 175, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\runners.py", line 109, in run
    return self._loop.run_until_complete(task)
    super().run_forever()
    ^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\base_events.py", line 604, in run_forever
    self._run_once()
    ^^^^^^^^^^^^^^^^
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\base_events.py", line 1873, in _run_once
    event_list = self._selector.select(timeout)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\windows_events.py", line 439, in select
    self._poll(timeout)
    ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\gvanrossum\pr-32105\Lib\asyncio\windows_events.py", line 808, in _poll
    status = _overlapped.GetQueuedCompletionStatus(self._iocp, ms)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt
Catching Generator Exit
Task was destroyed but it is pending!
task: <Task cancelling name='Task-1' coro=<looper() done, defined at C:\Users\gvanrossum\pr-32105\t.py:3> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[gather.<locals>._done_callback() at C:\Users\gvanrossum\pr-32105\Lib\asyncio\tasks.py:759]>
^CTerminate batch job (Y/N)? y
PS C:\Users\gvanrossum\pr-32105>

This does not seem to be a big improvement over the original issue. :-(

[1st1]

Why is this merged? No core dev reviewed this. Please revert.

[davidhewitt]

This appears like it might have caused breakage in the latest alpha: PyO3/pyo3#2314 (comment)

[gvanrossum]

This appears like it might have caused breakage in the latest alpha: PyO3/pyo3#2314 (comment)

I find the discussion in that issue hard to follow. Sure, something that happened between a6 and a7 broke PyO3. But what? Why would this change (which only affects how KeyboardInterrupt is handled) be the root cause?

[davidhewitt]

@gvanrossum sorry for my short comment earlier; was up in the middle of the night helping my wife feed my newborn :)

I haven't had chance to repro and poke around with this this myself, however I think the cause is possibly because the signal and threading modules are disagreeing about what is the "main thread" in PyO3's unit test suite. (Rust binary embedding Python via dynamic linking to libpython.)

The line erroring is the runners.py line 103 introduced in this patch, which installs the sigint handler:

cpython/Lib/asyncio/runners.py

Line 103 in 2e49bd0

signal.signal(signal.SIGINT, sigint_handler)

It reaches this error in the C:

cpython/Modules/signalmodule.c

Lines 533 to 535 in b6d5e3c

	_PyErr_SetString(tstate, PyExc_ValueError,
	"signal only works in main thread "
	"of the main interpreter");

That runners.py line 103 is guarded by a check threading.current_thread() is threading.main_thread(), hence my guess that threading / signal disagree what the main thread is in this case.

---

The PyO3 unit test suite tickling this crash can roughly be summarised as:

• Rust binary embedding Python (dynamic linking to libpython)
• The Python interpreter is started with Py_InitializeEx(0) (0 means don't register signal handlers)
• A different native thread acquires the GIL with PyGILState_Ensure()
• That thread then uses PyEval_EvalCode to run a snippet which contains a call to asyncio.run

[gvanrossum]

Sorry, it looks like you just answered at least part of the question I asked on the PyO3 issue. Anyway, it looks like we need a better check for determining whether it is safe to call signal(). Or perhaps we just need to put a try/except around that call?

[davidhewitt]

Thinking on this further, I suspect that threading check passes in PyO3's case above because running this code snippet is probably the first time threading is imported. That would explain why threading module thinks the thread is the main thread.

I see in signalmodule.c there's a symbol _PyOS_IsMainThread, which looks like the correct check (used in C) for whether signal.signal can be called. Do you think it makes sense to expose this as a Python API, perhaps signal.can_handle_signals()?

[gvanrossum]

@davidhewitt We're 1-2 weeks from feature freeze, and I don't have time to propose and implement a new API for this purpose (simple though it may seem, there are always loose ends, tests to write, etc.).

What I can do (other than rolling back @asvetlov's PR, which would be a shame) is add a try/except around the signal() call that catches the ValueError. That would presumably unblock you. (Things would go even faster if you proposed a PR -- I can just merge it.)

[gvanrossum]

(Either way you should probably open a new issue about this rather than keep chatting on this closed PR.)

[davidhewitt]

👍 I'll do my best to find time to write a fresh issue and PR at the weekend. Thanks for the help!