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

Inconsistent gather with child exception #81884

Closed
DmitriiIvaniushin mannequin opened this issue Jul 29, 2019 · 7 comments
Closed

Inconsistent gather with child exception #81884

DmitriiIvaniushin mannequin opened this issue Jul 29, 2019 · 7 comments
Labels
3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@DmitriiIvaniushin
Copy link
Mannequin

DmitriiIvaniushin mannequin commented Jul 29, 2019

BPO 37703
Nosy @asvetlov, @1st1, @miss-islington, @vinay0410
PRs
  • bpo-37703: improve asyncio.gather documentation regarding cancellation #15312
  • [3.9] bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312) #21557
  • [3.8] bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312) #21558
  • [3.7] bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312) #21559
  • Files
  • gather_cancel_doc.patch
  • gather_cancel_code.patch
  • 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 2020-07-20.08:49:34.904>
    created_at = <Date 2019-07-29.08:50:09.214>
    labels = ['type-bug', '3.7', 'expert-asyncio']
    title = 'Inconsistent gather with child exception'
    updated_at = <Date 2020-07-20.09:01:49.086>
    user = 'https://bugs.python.org/DmitriiIvaniushin'

    bugs.python.org fields:

    activity = <Date 2020-07-20.09:01:49.086>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-07-20.08:49:34.904>
    closer = 'asvetlov'
    components = ['asyncio']
    creation = <Date 2019-07-29.08:50:09.214>
    creator = 'Dmitrii Ivaniushin'
    dependencies = []
    files = ['48547', '48548']
    hgrepos = []
    issue_num = 37703
    keywords = ['patch']
    message_count = 7.0
    messages = ['348603', '349852', '349856', '349863', '373991', '373992', '373994']
    nosy_count = 5.0
    nosy_names = ['asvetlov', 'yselivanov', 'miss-islington', 'Dmitrii Ivaniushin', 'vinay0410']
    pr_nums = ['15312', '21557', '21558', '21559']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37703'
    versions = ['Python 3.7']

    @DmitriiIvaniushin
    Copy link
    Mannequin Author

    DmitriiIvaniushin mannequin commented Jul 29, 2019

    I found some issue that I suppose is a bug.

    Let us have long running coroutines. We use them in gather, and one of them raises an error. Since then we cannot cancel the gather anymore, thus remaining children are not cancelable and executed until complete or raise an exception themselves.

    ===

    import asyncio
    
    
    async def coro_with_error():
        # Coro raises en error with 1 sec delay
        await asyncio.sleep(1)
        raise Exception('Error in coro')
    
    
    async def cancellator(coro):
        # We use this to cancel gather with delay 1 sec
        await asyncio.sleep(1)
        coro.cancel()
    
    
    async def success_long_coro():
        # Long running coro, 2 sec
        try:
            await asyncio.sleep(2)
            print("I'm ok!")
            return 42
        except asyncio.CancelledError:
            # Track that this coro is really cancelled
            print('I was cancelled')
            raise
    
    
    async def collector_with_error():
        gather = asyncio.gather(coro_with_error(), success_long_coro())
        try:
            await gather
        except Exception:
            print(f"WHOAGH ERROR, gather done={gather.done()}")
            print(f'EXC={type(gather.exception()).__name__}')
            # We want to cancel still running success_long_coro()
            gather.cancel()
    
    
    async def collector_with_cancel():
        # Gather result from success_long_coro()
        gather = asyncio.gather(success_long_coro())
        # schedule cancel in 1 sec
        asyncio.create_task(cancellator(gather))
        try:
            await gather
        except Exception:
            print(f"WHOAGH ERROR, gather done={gather.done()}")
            print(f'EXC={type(gather.exception()).__name__}')
            # We want to cancel still running success_long_coro()
            gather.cancel()
            return
    
    # First case, cancel gather when children are running
    print('First case')
    loop = asyncio.get_event_loop()
    loop.create_task(collector_with_cancel())
    # Ensure test coros we fully run
    loop.run_until_complete(asyncio.sleep(3))
    print('Done')
    
    # Second case, cancel gather when child raise error
    print('Second case')
    loop = asyncio.get_event_loop()
    loop.create_task(collector_with_error())
    # Ensure test coros we fully run
    loop.run_until_complete(asyncio.sleep(3))
    print('Done')

    ===

    Actual output:

    First case
    I was cancelled
    WHOAGH ERROR, gather done=True
    EXC=CancelledError
    Done
    Second case
    WHOAGH ERROR, gather done=True
    EXC=Exception
    I'm ok!
    Done
    

    Expected output:

    First case
    I was cancelled
    WHOAGH ERROR, gather done=True
    EXC=CancelledError
    Done
    Second case
    I was cancelled
    WHOAGH ERROR, gather done=True
    EXC=Exception
    Done
    

    Documentations says:

    If gather() is cancelled, all submitted awaitables (that have not completed yet) are also cancelled.
    But it mentions no cases on child coros' exceptions.

    From doc:

    If return_exceptions is False (default), the first raised exception is immediately propagated to the task that awaits on gather(). Other awaitables in the aws sequence won’t be cancelled and will continue to run.
    Which is true, exception is propagated, the gather has an exception set, marked as done() so its children are not cancelled.

    I believe asyncio should allow cancellation in that case.

    @DmitriiIvaniushin DmitriiIvaniushin mannequin added 3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error labels Jul 29, 2019
    @vinay0410
    Copy link
    Mannequin

    vinay0410 mannequin commented Aug 16, 2019

    Hi Dimitri,
    You are right, gather.cancel() doesn't work once it has propagated an exception. This happens because after propagating the exception to the caller, gather is marked as done. And cancel doesn't work after a Future object has been marked done.
    You can test the same by printing the return value of gather.cancel(). It will be False

    I also believe that the documentation of gather should explicitly mention this. But, depending on the fact, whether this is an expected behaviour, current code base might also need changes.

    Therefore I have created two patches, one updating the current documentation according to the current functionality, and other changing the codebase which supports cancelling even after raising exceptions.

    I will try to contact one of the core developers on deciding which one is the way to go.

    @asvetlov
    Copy link
    Contributor

    The behavior change is not backward compatible, we cannot apply it.

    Also, please use github pull requests for proposing a patch: https://devguide.python.org/pullrequest/

    @vinay0410
    Copy link
    Mannequin

    vinay0410 mannequin commented Aug 16, 2019

    Hi Andrew,
    Thanks for replying!

    I understand that the behavior change is not backward compatible. But the current behavior is a bit ambiguous ad mentioned by Dimitri, therefore I have updated the documentation and opened a Pull request.

    I would be very glad if you could review it.
    Thanks!

    Also, would adding an optional argument like force_cancel=False to gather.cancel, which won't return even if gather is done be backward compatible. Should I open a PR with this change also, so that you can better review it and see if it looks good.

    @miss-islington
    Copy link
    Contributor

    New changeset d42528a by Vinay Sharma in branch 'master':
    bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
    d42528a

    @miss-islington
    Copy link
    Contributor

    New changeset 58f59a9 by Miss Islington (bot) in branch '3.8':
    bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
    58f59a9

    @miss-islington
    Copy link
    Contributor

    New changeset 46634b7 by Miss Islington (bot) in branch '3.9':
    bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
    46634b7

    @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
    3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants