This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author njs
Recipients asvetlov, njs, yselivanov
Date 2019-09-22.08:45:23
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1569141924.07.0.413373545347.issue38248@roundup.psfhosted.org>
In-reply-to
Content
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 :-)
History
Date User Action Args
2019-09-22 08:45:24njssetrecipients: + njs, asvetlov, yselivanov
2019-09-22 08:45:24njssetmessageid: <1569141924.07.0.413373545347.issue38248@roundup.psfhosted.org>
2019-09-22 08:45:24njslinkissue38248 messages
2019-09-22 08:45:23njscreate