Confusing error handling for KeyboardInterrupt

[sametmax]

I reported this issue on the Python tracker but got no answer.

If you trigger KeyboardInterrupt in a coroutine and catch it, the program terminates cleanly:

import asyncio

async def bar():
 raise KeyboardInterrupt

loop = asyncio.get_event_loop()

try:
 loop.run_until_complete(bar())
except KeyboardInterrupt:
 print("It's ok")
finally:
 loop.stop()
 loop.close()

This outputs:

It's ok

However, if you wrap the coroutine in a Task, you will get a mixed behavior:

try:
 task = asyncio.ensure_future(bar())
 loop.run_until_complete(task)
except KeyboardInterrupt:
 print("It's ok")

This outputs:

It's ok
Task exception was never retrieved
future: <Task finished coro=<bar() done, defined at ki_bug.py:4> exception=KeyboardInterrupt()>
Traceback (most recent call last):
 File "ki_bug.py", line 10, in <module>
 loop.run_until_complete(main_future)
 File "/usr/lib/python3.5/asyncio/base_events.py", line 325, in run_until_complete
 self.run_forever()
 File "/usr/lib/python3.5/asyncio/base_events.py", line 295, in run_forever
 self._run_once()
 File "/usr/lib/python3.5/asyncio/base_events.py", line 1258, in _run_once
 handle._run()
 File "/usr/lib/python3.5/asyncio/events.py", line 125, in _run
 self._callback(*self._args)
 File "/usr/lib/python3.5/asyncio/tasks.py", line 239, in _step
 result = coro.send(None)
 File "ki_bug.py", line 5, in bar
 raise KeyboardInterrupt
KeyboardInterrupt

We have several contradictory behaviors: the KeyboardInterrupt is raised, and captured by the future (since your can do task.exception() to suppress the stracktrace) but also catched by except while the program is allowed to continue, yet still the stack trace is displayed and eventually the program return code will be 0.

It's very confusing.

I believe it's because exceptions don't break out the event loop, but KeyboardInterrupt is having some kind special treatment.

[vxgmichel]

There is a bit of explanation in BaseEventLoop.run_until_complete:

 try:
 self.run_forever()
 except:
 if new_task and future.done() and not future.cancelled():
 # The coroutine raised a BaseException. Consume the exception
 # to not log a warning, the caller doesn't have access to the
 # local task.
 future.exception()
 raise

If the exception is an instance of BaseException but not Exception (such as KeyboardInterrupt), it bubbles up and reaches this try-except. If the task has been created inside run_until_complete (new_task is True), then the exception is consumed since there is no way to access the task before it is garbage collected.

The problem you reported could easily be fixed by removing new_task in the test. However, the raised BaseException might be different from the exception set in the task, so I guess it is more of a feature than a bug. Another way to address this issue would be to compare the exceptions:

 try:
 self.run_forever()
 except BaseException as exc:
 if future._exception is exc \
 # Is this line still necessary?
 or new_task and future.done() and not future.cancelled():
 # The coroutine raised a BaseException. Consume the exception
 # to not log a warning, the caller doesn't have access to the
 # local task.
 future.exception()
 raise

However, this part of the code is very central, so I guess any change here is a pretty big deal.

[sametmax]

Thank you.

We manage to live with it, it's just debugging will confuse the hell out of new comers. Asyncio is pretty hard to understand, and even more to debug, so I believe we should do everything to make it easier. I have been working on an asyncio project for some month now, and I must say there is no way an average programmer would have the patience to dig into all the debugging sessions we went through.

[gvanrossum]

It has been suggested that asyncio should properly catch and deliver all exceptions, even BaseException. I am beginning to think that we should really do that! There's even an old pull request to do it: #305 Probably needs updating and I worry that it would break other things (that's why the PR hasn't been merged yet). But still sounds like it would generally be an improvement. @asvetlov What do you think? Would aiohttp need changes if we did this?

[vxgmichel]

@gvanrossum But doesn't it make sense for a KeyboardInterrupt to bubble up to loop.run_forever and stop the loop regardless of where the exception has been raised?

I'm thinking about how the PR #305 might break the servers relying on Ctrl+C to stop their execution. Consider the TCP echo server using streams from the asyncio documentation. If a KeyboardInterrupt is raised while waiting for a client to connect (in loop._run_once), the execution will stop as expected. But if the KeyboardInterrupt is raised while dealing with a client request (in the handle_echo coroutine), the loop will not stop ( and a Task exception was never retrieved message will be logged). This inconsistent behavior seems like a pretty serious issue.

[gvanrossum]

That's the kind of thing we need to solve before accepting the PR. Do you
have a suggestion for a fix?

On Sunday, July 31, 2016, Vincent Michel notifications@github.com wrote:

@gvanrossum https://github.com/gvanrossum But doesn't it make sense for
a KeyboardInterrupt to bubble up to loop.run_forever and stop the loop
regardless of where the exception has been raised?

I'm thinking about how the PR #305
#305 might break the servers
relying on Ctrl+C to stop their execution. Consider the TCP echo server
using streams
https://docs.python.org/3.4/library/asyncio-stream.html#tcp-echo-server-using-streams
from the asyncio documentation. If a KeyboardInterrupt is raised while
waiting for a client to connect (in loop._run_once), the execution will
stop as expected. But if the KeyboardInterrupt is raised while dealing
with a client request (in the handle_echo coroutine), the loop will not
stop ( and a Task exception was never retrieved message will be logged).
This inconsistent behavior seems like a pretty serious issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#341 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACwrMgNweN2dEIArfWBzze_StbYddxSjks5qbIa_gaJpZM4IWvsV
.

--Guido (mobile)

[vxgmichel]

@gvanrossum
I like @Haypo's idea of setting a handler for SIGINT. A keyboard interrupt is an asynchronous event, and it makes a lot of sense to let the event loop deal with it. It also enforces the idea that the code between two await statements is safe and will never be interrupted.

The default SIGINT handler could stop the loop, and make sure that a KeyboardInterrupt exception is raised in loop.run_forever so the application can react and let the running coroutines terminate properly.

You mentioned the fact that a SIGINT won't be able to interrupt a tight CPU loop, but I don't really see that as a problem.

[asvetlov]

@gvanrossum I don't see any harm from #305
aiohttp (and all other libraries supported by me) definitely doesn't change own behavior after PR appliance.

[1st1]

You mentioned that a SIGINT won't be able to interrupt a tight CPU loop, but I don't really see that as a problem.

@vxgmichel This is a big no-no. In the first version of uvloop I did exactly this -- handle SIGINT and let the loop to handle it asynchronously. It was completely unusable. Turns out people write tight loops quite frequently, and inability to stop your Python program with Ctrl-C is something they aren't prepared to handle at all. Now I'm using PyErr_SetInterrupt to actually interrupt any Python code on SIGINT to replicate asyncio behaviour.

[gvanrossum]

I think the problem with the TCP server example can be fixed, with some care. The example hangs with a similar error if the handler raises a non-blocking exception (without the BaseException PR), and I think the problem is that asyncio.start_server() is a bit naive: it creates a Task and then forgets about it. It should at the very least collect errors from the Tasks it has created, and that error-catching code could special-case KeyboardInterrupt. Also, assuming you're not being DoS'ed with continued connect requests, hitting ^C again _will_ exit the loop.

[vxgmichel]

I crafted another example to demonstrate that some programs might get non-interruptible after PR #305. In this example, the background task is most likely to catch and ignore the first Ctrl-C (the second one will interrupt the program though).

@1st1

[...] inability to stop your Python program with Ctrl-C is something they aren't prepared to handle at all.

That is also what gets me worried about PR #305. I'd sum it up this way:

Ctrl-C stops the loop, ...

• [master] ... but asyncio internals might get corrupted if you try to recover.
• [PR Handle BaseException in event loop. #305] ... unless a background task is being executed when KeyboardInterrupt is raised.
• [PR Handle BaseException in event loop. #305 + SIGINT handler] ... unless there is synchronous code in your coroutines or callbacks.

[gvanrossum]

Then I would like to evolve #305 into something that always stops the loop on ^C, unless the application takes specific measures (e.g. installing a SIGINT handler, or perhaps a try/except that catches KeyboardInterrupt).

[sametmax]

There is an alternative possibility:

• catch all exceptions;
• add a system to stop the loop and reraise some exceptions;
• add a configuration system to chose the exceptions to catch and reraise. We should have hooks to create custom handlers for thoose;
• make the default conf values to be catching keyboard interrupt and reraise it;
• then document that and let the user choose the behavior;

Having a generic middleware system to handle exception and decide which one stop the loop, which one you log and which one you silent would be a nice thing anyway.

[1st1]

catch all exceptions;
add a system to stop the loop and reraise some exceptions;

This sounds right.

add a configuration system to chose the exceptions to catch and reraise. We should have hooks to create custom handlers for thoose;

I'm not sure I like this. What's the use case? What if you have two asyncio libs requiring different configs?

[sametmax]

Two async libs can already change the event loop, the policy and some the signal handlers. There is nothing to stop conflicting libs to screw with each others except documentation and good will.

The use case for this is the creation of a framework, and the definition of the error handling policy for this framework : logging, recovery, debugging, etc.

E.G:

On the project we are working now, we want to make debugging in dev mode very easy, especially for beginners. So we go to great lengths to detect common or weird errors and ensure the user can easily spot them, understand the problem and choose a fix.

Unfortunately, with the current architecture, this is complicated, we had to:

• create a fail fast mode so that if you activate it, any error just break the server and print a very verbose stack trace for debugging: https://github.com/Tygs/tygs/blob/master/src/tygs/app.py#L22
• install some convoluted error handling system : https://github.com/Tygs/tygs/blob/master/src/tygs/app.py#L113 and https://github.com/Tygs/tygs/blob/master/src/tygs/app.py#L156
• make sure you deal with exceptions that are not errors but workflow handling because in the web they are very common : https://github.com/Tygs/tygs/blob/master/src/tygs/components.py#L167
• deal with the fact anybody can hijack the loop and stop or start it when it pleases him/her : https://github.com/Tygs/tygs/blob/master/src/tygs/app.py#L84
• take the user by the hand and ensure we got the coroutines and awaitables where we need it : https://github.com/Tygs/tygs/blob/master/src/tygs/app.py#L84

And yet, while I think 30% of our unit tests are for those (https://github.com/Tygs/tygs/tree/master/tests), we have still some problems such as some errors being silent in unit tests with pytest.

And after all that, and 100% code coverage, we are maybe at 40% of the feature we expect for v0.1, as this took us many more iterations than we expected to get right. Not complaining, I'm glad we have asyncio in Python, and I'm super excited about being able to code this, but let's use the opportunity to make the next people's life easier.

Error handling, life cycle and debugging are really a key component to get right in asyncio, especially since the handling of the loop is manual, explicit and open to wind. NodeJS dev don't have to worry about half of that.

[1st1]

@sametmax Thanks for the explanation. I'll take a closer look at #305 (probably will have to be rewritten) in a few days.

And yet we have still some problems such as some errors being silent in unit tests with pytest.

Yeah, I saw that with pytest. Was wondering what's going on but didn't have time to investigate in detail.