classification
Title: Inconsistent gather with child exception
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Dmitrii Ivaniushin, asvetlov, miss-islington, vinay0410, yselivanov
Priority: normal Keywords: patch

Created on 2019-07-29 08:50 by Dmitrii Ivaniushin, last changed 2020-07-20 09:01 by miss-islington. This issue is now closed.

Files
File name Uploaded Description Edit
gather_cancel_doc.patch vinay0410, 2019-08-16 08:45
gather_cancel_code.patch vinay0410, 2019-08-16 08:46
Pull Requests
URL Status Linked Edit
PR 15312 merged vinay0410, 2019-08-16 13:52
PR 21557 merged miss-islington, 2020-07-20 08:43
PR 21558 merged miss-islington, 2020-07-20 08:43
PR 21559 closed miss-islington, 2020-07-20 08:43
Messages (7)
msg348603 - (view) Author: Dmitrii Ivaniushin (Dmitrii Ivaniushin) Date: 2019-07-29 08:50
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.
msg349852 - (view) Author: Vinay Sharma (vinay0410) * Date: 2019-08-16 08:45
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.
msg349856 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-08-16 12:02
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/
msg349863 - (view) Author: Vinay Sharma (vinay0410) * Date: 2019-08-16 14:03
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.
msg373991 - (view) Author: miss-islington (miss-islington) Date: 2020-07-20 08:43
New changeset d42528a3a2c7d79fd2e6c9f2a02f3ce12d44c8cc by Vinay Sharma in branch 'master':
bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
https://github.com/python/cpython/commit/d42528a3a2c7d79fd2e6c9f2a02f3ce12d44c8cc
msg373992 - (view) Author: miss-islington (miss-islington) Date: 2020-07-20 09:01
New changeset 58f59a962180123a6d29ece512d198b365726b33 by Miss Islington (bot) in branch '3.8':
bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
https://github.com/python/cpython/commit/58f59a962180123a6d29ece512d198b365726b33
msg373994 - (view) Author: miss-islington (miss-islington) Date: 2020-07-20 09:01
New changeset 46634b7aa82f014cd0039afb7f0ed860605beb9d by Miss Islington (bot) in branch '3.9':
bpo-37703: improve asyncio.gather documentation regarding cancellation (GH-15312)
https://github.com/python/cpython/commit/46634b7aa82f014cd0039afb7f0ed860605beb9d
History
Date User Action Args
2020-07-20 09:01:49miss-islingtonsetmessages: + msg373994
2020-07-20 09:01:02miss-islingtonsetmessages: + msg373992
2020-07-20 08:49:34asvetlovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-07-20 08:43:26miss-islingtonsetpull_requests: + pull_request20705
2020-07-20 08:43:19miss-islingtonsetpull_requests: + pull_request20704
2020-07-20 08:43:11miss-islingtonsetpull_requests: + pull_request20703
2020-07-20 08:43:06miss-islingtonsetnosy: + miss-islington
messages: + msg373991
2019-08-16 14:03:06vinay0410setmessages: + msg349863
2019-08-16 13:52:40vinay0410setstage: patch review
pull_requests: + pull_request15031
2019-08-16 12:02:40asvetlovsetmessages: + msg349856
2019-08-16 08:46:08vinay0410setfiles: + gather_cancel_code.patch
2019-08-16 08:45:17vinay0410setfiles: + gather_cancel_doc.patch

nosy: + vinay0410
messages: + msg349852

keywords: + patch
2019-07-29 08:50:09Dmitrii Ivaniushincreate