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

bpo-31033: Make traceback for cancelled asyncio tasks more useful #19951

Merged
merged 10 commits into from
May 18, 2020

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented May 6, 2020

When an asyncio.Task is cancelled, the exception traceback now starts with where the task was first interrupted.

Previously, the traceback only had "depth one," which wasn't too useful.

https://bugs.python.org/issue31033

@cjerdonek cjerdonek requested review from 1st1 and asvetlov as code owners May 6, 2020 11:06
@cjerdonek cjerdonek changed the title [WIP] bpo-31033: improved traceback for cancelled asyncio tasks bpo-31033: improved traceback for cancelled asyncio tasks May 6, 2020
@cjerdonek cjerdonek force-pushed the fix-issue-31033 branch 2 times, most recently from 936548a to ba37fb2 Compare May 7, 2020 14:18
@cjerdonek cjerdonek changed the title bpo-31033: improved traceback for cancelled asyncio tasks bpo-31033: Make traceback for cancelled asyncio tasks more useful May 7, 2020
@cjerdonek cjerdonek force-pushed the fix-issue-31033 branch 2 times, most recently from 94a8291 to b89ddef Compare May 8, 2020 10:48
@cjerdonek
Copy link
Member Author

cjerdonek commented May 13, 2020

@1st1 Yury, would you also be able to review / take a look at this one? I feel this is more important than PR #19979 for 3.9 from an asyncio perspective and is also simpler. (The PR's are compatible with one another.) Thank you in advance.

Lib/asyncio/futures.py Outdated Show resolved Hide resolved
cjerdonek added 6 commits May 15, 2020 17:05
This moves (and renames) exc_state_clear() in genobject.c to
_PyErr_ClearExcState() in pycore_pyerrors.h.
This simplifies calling _PyErr_ChainExceptions() when we want to
set a _PyErr_StackItem as the exception context.
When an asyncio.Task is cancelled, the exception traceback now
starts with where the task was first interrupted.  Previously,
the traceback only had "depth one."
@cjerdonek cjerdonek force-pushed the fix-issue-31033 branch 2 times, most recently from cf2561f to e4cfc90 Compare May 16, 2020 12:51
@cjerdonek
Copy link
Member Author

@1st1 Here's a new version of the PR after rebasing with PR #19979 and ensuring that the cancelled message isn't duplicated multiple times in the traceback. I'll show you what the output looks like in a comment after this.

I wasn't able to get the tests passing using a weakref or setting self._cancelled_exc to None like you suggested. It might not be possible in general. When a weakref is used in Task with CFuture, the exception gets deallocated here before the CancelledError ever has a chance to be constructed:

except BaseException as exc:
# This may also be a cancellation.
self.__step(exc)
else:

Similarly, setting self._cancelled_exc to None wasn't working in certain tests for different reasons.

Can this be an optimization that's considered later?

@cjerdonek
Copy link
Member Author

Here's an example to show you what the output look like. For this code:

import asyncio, sys

async def nested():
    fut = asyncio.sleep(1)
    await fut  # AWAIT #1

async def run():
    task = asyncio.create_task(nested())
    await asyncio.sleep(0)
    task.cancel('POSSIBLY LONG CANCEL MESSAGE')

    await task  # AWAIT #2

loop = asyncio.new_event_loop()
try:
    loop.run_until_complete(run())
finally:
    loop.close()

Here is before the PR:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 17, in <module>
    loop.run_until_complete(run())
  File "/.../cpython/Lib/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
asyncio.exceptions.CancelledError: POSSIBLY LONG CANCEL MESSAGE

And here is after the PR:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 6, in nested
    await fut  # AWAIT #1
  File "/.../cpython/Lib/asyncio/tasks.py", line 669, in sleep
    return await future
asyncio.exceptions.CancelledError: POSSIBLY LONG CANCEL MESSAGE

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 13, in run
    await task  # AWAIT #2
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 17, in <module>
    loop.run_until_complete(run())
  File "/.../cpython/Lib/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
asyncio.exceptions.CancelledError

@cjerdonek
Copy link
Member Author

Okay, I was able to get it working with setting self._cancelled_exc to None. This is done in _make_cancelled_error() in the Python code and in _asyncio_Future__make_cancelled_error_impl() in the C code.

@cjerdonek
Copy link
Member Author

@1st1 Is there any way this can be merged by tomorrow? I addressed your first review comment (setting self._cancelled_exc to None when it's no longer needed). The other one (weakref) I don't think is compatible with the feature (which I can explain more fully).

Lib/asyncio/futures.py Outdated Show resolved Hide resolved
Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

Chris, this looks good and the usability improvement is huge. I've left a nit comment, feel free to address it in this PR or later, up to you. Thanks!

@1st1
Copy link
Member

1st1 commented May 18, 2020

@ambv I think this needs to be included in 3.9. I'm going to merge it myself tomorrow morning if Chris doesn't. Or you can merge it as is!

@cjerdonek cjerdonek merged commit da742ba into python:master May 18, 2020
@cjerdonek cjerdonek deleted the fix-issue-31033 branch May 18, 2020 05:47
cjerdonek added a commit to cjerdonek/cpython that referenced this pull request May 18, 2020
_PyErr_ChainStackItem was just added in pythonGH-19951 (for bpo-31033).
cjerdonek added a commit that referenced this pull request May 18, 2020
ambv pushed a commit that referenced this pull request May 19, 2020
_PyErr_ChainStackItem was just added in GH-19951 (for bpo-31033).
(cherry picked from commit ff7a8b0)

Co-authored-by: Chris Jerdonek <chris.jerdonek@gmail.com>

Co-authored-by: Chris Jerdonek <chris.jerdonek@gmail.com>
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
…H-19951)

When an asyncio.Task is cancelled, the exception traceback now
starts with where the task was first interrupted.  Previously,
the traceback only had "depth one."
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
_PyErr_ChainStackItem was just added in pythonGH-19951 (for bpo-31033).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants