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

inconsistency in asyncio.Task between cancellation while running vs. cancellation immediately after it finishes #82429

Closed
njsmith opened this issue Sep 22, 2019 · 7 comments

Comments

@njsmith
Copy link
Contributor

njsmith commented Sep 22, 2019

BPO 38248
Nosy @vstinner, @njsmith, @asvetlov, @1st1, @willingc
PRs
  • bpo-38248: Fix inconsistent immediate asyncio.Task cancellation #16330
  • [3.8] bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330) #16383
  • [3.8] bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330) #16383
  • bpo-38321: Fix _asynciomodule.c compiler warning #16493
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-10-01.16:01:15.373>
    created_at = <Date 2019-09-22.08:45:24.028>
    labels = []
    title = 'inconsistency in asyncio.Task between cancellation while running vs. cancellation immediately after it finishes'
    updated_at = <Date 2019-10-01.16:01:15.372>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2019-10-01.16:01:15.372>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-10-01.16:01:15.373>
    closer = 'yselivanov'
    components = []
    creation = <Date 2019-09-22.08:45:24.028>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38248
    keywords = ['patch']
    message_count = 7.0
    messages = ['352964', '352987', '353165', '353182', '353578', '353678', '353686']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'njs', 'asvetlov', 'yselivanov', 'willingc']
    pr_nums = ['16330', '16383', '16383', '16493']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue38248'
    versions = []

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Sep 22, 2019

    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 :-)

    @1st1
    Copy link
    Member

    1st1 commented Sep 22, 2019

    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.

    @willingc
    Copy link
    Contributor

    New changeset edad4d8 by Carol Willing (Yury Selivanov) in branch 'master':
    bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330)
    edad4d8

    @willingc
    Copy link
    Contributor

    New changeset 16cec13 by Carol Willing (Miss Islington (bot)) in branch '3.8':
    bpo-38248: Fix inconsistent immediate asyncio.Task cancellation (GH-16330) (GH-16383)
    16cec13

    @vstinner
    Copy link
    Member

    New changeset efe74b6 by Victor Stinner in branch 'master':
    bpo-38321: Fix _asynciomodule.c compiler warning (GH-16493)
    efe74b6

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2019

    New changeset bfe1f74 by Victor Stinner in branch '3.8':
    [3.8] bpo-3832: Fix compiler warnings (GH-16518)
    bfe1f74

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2019

    Can this issue be closed now?

    @1st1 1st1 closed this as completed Oct 1, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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

    No branches or pull requests

    4 participants