jfhbrook/pyee

asyncio done callback is also called for cancelled events

mjpieters opened this issue · 2 comments

pyee registers a 'done' callback to asyncio tasks / futures, which retrieves any exceptions to emit:

pyee/pyee/_asyncio.py

Lines 55 to 59 in 8cab87d

@f.add_done_callback
def _callback(f):
exc = f.exception()
if exc:
self.emit('error', exc)

However, if the task / future is cancelled, then calling .exception() will raise asyncio.CancelledError, see the asyncio.Task.exception() documentation:

If the Task has been cancelled, this method raises a CancelledError exception.

During a normal shutdown, I see a lot of these:

Traceback (most recent call last):
    ...
KeyboardInterrupt

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.8/asyncio/events.py", line 81, in _run
    self._context.run(self._callback, *self._args)
  File "/.../site-packages/pyee/_compat.py", line 60, in _callback
    exc = f.exception()
asyncio.exceptions.CancelledError

(this is via a 3rd-party project that uses pyee and hasn't switched yet AsyncIOEventEmitter, but the principle is the same).

In my own callback handlers, I always check first if the task has been cancelled, and further ignore KeyboardInterrupt and SystemExit:

def foobar_sample_handler(task: asyncio.Task[Any]) -> None:
    if task.cancelled():
        return
    exception = task.exception()
    if exception is None or isinstance(exception, (KeyboardInterrupt, SystemExit)):
        return
    # ... do stuff with exception.

Does pyee really need to propagate KeyboardInterrupt and SystemExit, as well? It definitely should not trip up on cancelled tasks, however.

I tbh didn't consider cancelations when building out the asyncio functionality.

I'd be open to sketching out an updated design that sends cancellation errors to a "canceled" event which doesn't throw unhandled cancelations. It looks like this API is explicitly designed to be captured and handled at its source, so I think this makes sense.

I'm less interested in special default handling for KeyboardInterrupt and SystemExit, since those are afaik intended to propagate to the outermost scope. That said, I'd look at a POC that adds configurable handling for these events, with a preference towards an API that allows for general purpose ignoring of certain errors.

Would you be open to sending me some pull requests? I think we can both agree that the actual code changes required for these proposed features are a relatively low lift, and a concrete implementation would give us something to talk about.

I'm sorry, I don't really have time to spare right now.

I consider this a bug report, at any rate, cancelled tasks should not lead to unhandled exceptions. Adding a new 'cancelled' event is really a new feature.

You are right, ignoring KeyboardInterrupt and SystemExit is application-specific, my callback is there just to record abnormal exits and those two exceptions, in my case, should be treated as normal. If pyee aims to make all exceptions available as events then you should not ignore them here.