classification
Title: inconsistency in asyncio.Task between cancellation while running vs. cancellation immediately after it finishes
Type: Stage: resolved
Components: Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, njs, vstinner, willingc, yselivanov
Priority: normal Keywords: patch

Created on 2019-09-22 08:45 by njs, last changed 2019-10-01 16:01 by yselivanov. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16330 merged yselivanov, 2019-09-22 20:07
PR 16383 merged miss-islington, 2019-09-25 10:32
PR 16383 merged miss-islington, 2019-09-25 10:32
PR 16493 merged vstinner, 2019-09-30 14:21
Messages (7)
msg352964 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2019-09-22 08:45
Just noticed this while looking at the code to asyncio.Task.__step

asyncio Futures have two different error states: they can have an arbitrary exception set, or they can be marked as cancelled.

asyncio Tasks handle this by detecting what exception was raised by the task code: if it's a CancelledError, then the mark the Future as cancelled, and if it's anything else, they set that as the Future's exception.

There is also a special case handled inside the 'except StopIteration' clause in Task.__step. If we request that a Task be cancelled, but then the task exits normally before we have a chance to throw in a CancelledError, then we also want mark the Future as cancelled. BUT, this special case is handled incorrectly: it does Future.set_exception(CancelledError()), instead of Future.cancel(). Normally it's impossible for a task to end up with its exception set to CancelledError, but it does happen in this one weird edge case, basically as a race condition.

Here's some sample code to illustrate the problem (tested on 3.7):

------

import asyncio

# This gets cancelled normally
async def cancel_early():
    asyncio.current_task().cancel()
    await asyncio.sleep(1)

async def cancel_late():
    asyncio.current_task().cancel()
    # No sleep, so CancelledError doesn't get delivered until after the
    # task exits

async def main():
    t_early = asyncio.create_task(cancel_early())
    await asyncio.sleep(0.1)
    print(f"t_early.cancelled(): {t_early.cancelled()!r}")

    t_late = asyncio.create_task(cancel_late())
    await asyncio.sleep(0.1)
    print(f"t_late.cancelled(): {t_late.cancelled()!r}")
    print(f"t_late.exception(): {t_late.exception()!r}")

asyncio.run(main())


------

Output:

t_early.cancelled(): True
t_late.cancelled(): False
t_late.exception(): CancelledError()

The obvious fix would be to modify the 'except StopIteration' branch to handle this case by calling super().cancel() instead of super().set_exception(...).

Alternatively, I could see an argument that asyncio.Task should always preserve the CancelledError, so that e.g. you can get a proper traceback. In that case we'd need to change the 'except CancelledError' branch to call super().set_exception(...) instead of super().cancel(). This would also need some more changes, like overriding .cancelled() to check for a stored exception and then return isinstance(stored_exc, CancelledError), and maybe others... I'm not sure of the full consequences.

But handling these two cases differently is definitely wrong, that part I'm sure of :-)
msg352987 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-09-22 18:43
> The obvious fix would be to modify the 'except StopIteration' branch to handle this case by calling super().cancel() instead of super().set_exception(...).

Yeah, I think this is the solution we should do in 3.8.
msg353165 - (view) Author: Carol Willing (willingc) * (Python committer) Date: 2019-09-25 10:32
New changeset edad4d89e357c92f70c0324b937845d652b20afd by Carol Willing (Yury Selivanov) in branch 'master':
bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330)
https://github.com/python/cpython/commit/edad4d89e357c92f70c0324b937845d652b20afd
msg353182 - (view) Author: Carol Willing (willingc) * (Python committer) Date: 2019-09-25 11:49
New changeset 16cec136b75daf438080a5b6685d2679dfa406af by Carol Willing (Miss Islington (bot)) in branch '3.8':
bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330) (GH-16383)
https://github.com/python/cpython/commit/16cec136b75daf438080a5b6685d2679dfa406af
msg353578 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-30 14:41
New changeset efe74b6369a8d08f27c69703fcc1686966e51068 by Victor Stinner in branch 'master':
bpo-38321: Fix _asynciomodule.c compiler warning (GH-16493)
https://github.com/python/cpython/commit/efe74b6369a8d08f27c69703fcc1686966e51068
msg353678 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 11:52
New changeset bfe1f74e39d0049a829962050e86a6a2d2a2781e by Victor Stinner in branch '3.8':
[3.8] bpo-3832: Fix compiler warnings (GH-16518)
https://github.com/python/cpython/commit/bfe1f74e39d0049a829962050e86a6a2d2a2781e
msg353686 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 11:56
Can this issue be closed now?
History
Date User Action Args
2019-10-01 16:01:15yselivanovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-10-01 11:56:33vstinnersetmessages: + msg353686
2019-10-01 11:52:03vstinnersetmessages: + msg353678
2019-09-30 14:41:39vstinnersetnosy: + vstinner
messages: + msg353578
2019-09-30 14:21:23vstinnersetpull_requests: + pull_request16081
2019-09-25 11:49:04willingcsetmessages: + msg353182
2019-09-25 10:32:48miss-islingtonsetpull_requests: + pull_request15964
2019-09-25 10:32:22miss-islingtonsetpull_requests: + pull_request15963
2019-09-25 10:32:13willingcsetnosy: + willingc
messages: + msg353165
2019-09-22 20:07:36yselivanovsetkeywords: + patch
stage: patch review
pull_requests: + pull_request15907
2019-09-22 18:43:29yselivanovsetmessages: + msg352987
2019-09-22 08:45:24njscreate