Skip to content

bpo-40607: Reraise exception during task cancelation in asyncio.wait_for() #20054

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

Merged
merged 1 commit into from
May 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Doc/library/asyncio-task.rst
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,8 @@ Timeouts
wrap it in :func:`shield`.

The function will wait until the future is actually cancelled,
so the total wait time may exceed the *timeout*.
so the total wait time may exceed the *timeout*. If an exception
happens during cancellation, it is propagated.

If the wait is cancelled, the future *aw* is also cancelled.

Expand Down
10 changes: 9 additions & 1 deletion Lib/asyncio/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,15 @@ async def wait_for(fut, timeout, *, loop=None):
# after wait_for() returns.
# See https://bugs.python.org/issue32751
await _cancel_and_wait(fut, loop=loop)
raise exceptions.TimeoutError()
# In case task cancellation failed with some
# exception, we should re-raise it
Comment on lines +499 to +500
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case when task cancellation failed with some result (not exception)? Is it OK to ignore it? I mean something like

async def inner():
    try:
        await asyncio.sleep(0.2)
    except asyncio.CancellationError:
        return 123  # instead of `raise FooException`

# See https://bugs.python.org/issue40607
try:
fut.result()
except exceptions.CancelledError as exc:
raise exceptions.TimeoutError() from exc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put also an else: raise TimeoutError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I think that else branch is unreachable: if there isn't any exception in try block, then return works normally and we leave this method. Please explain if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but your new code is simply not equivalent to the previous code that raised TimeoutError unconditionally. Now I see that there might be a condition for clear return and I don't want that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked issue talks about the correct exception propagation. Basically, wait_for timeouts with an exception and we want that original exception to be propagated. That's fine and I'm +1 for that.

The code here is slightly different though and a bit hard to trace so it would be easier to read if it was replaced with:

           try:
                fut.result()
            except exceptions.CancelledError as exc:
                raise exceptions.TimeoutError() from exc
            else:
                raise exceptions.TimeoutError()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is that there is a race condition where the task can complete after the timeout has elapsed. So the question is whether the caller should get to learn that the task completed in this case.

Copy link
Member

@1st1 1st1 May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the current implementation discards the result, as the probability of actually having it is low. I'd like to keep it that way. If we want to change the current behavior that should be done in a separate issue, with a new PR and new discussion.

else:
raise exceptions.TimeoutError()
finally:
timeout_handle.cancel()

Expand Down
55 changes: 51 additions & 4 deletions Lib/test/test_asyncio/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ def __await__(self):
return self


# The following value can be used as a very small timeout:
# it passes check "timeout > 0", but has almost
# no effect on the test performance
_EPSILON = 0.0001


class BaseTaskTests:

Task = None
Expand Down Expand Up @@ -877,12 +883,53 @@ async def inner():

inner_task = self.new_task(loop, inner())

with self.assertRaises(asyncio.TimeoutError):
await asyncio.wait_for(inner_task, timeout=0.1)
await asyncio.wait_for(inner_task, timeout=_EPSILON)

self.assertTrue(task_done)
with self.assertRaises(asyncio.TimeoutError) as cm:
loop.run_until_complete(foo())

loop.run_until_complete(foo())
self.assertTrue(task_done)
chained = cm.exception.__context__
self.assertEqual(type(chained), asyncio.CancelledError)

def test_wait_for_reraises_exception_during_cancellation(self):
loop = asyncio.new_event_loop()
self.addCleanup(loop.close)

class FooException(Exception):
pass

async def foo():
async def inner():
try:
await asyncio.sleep(0.2)
finally:
raise FooException

inner_task = self.new_task(loop, inner())

await asyncio.wait_for(inner_task, timeout=_EPSILON)

with self.assertRaises(FooException):
loop.run_until_complete(foo())

def test_wait_for_raises_timeout_error_if_returned_during_cancellation(self):
loop = asyncio.new_event_loop()
self.addCleanup(loop.close)

async def foo():
async def inner():
try:
await asyncio.sleep(0.2)
except asyncio.CancelledError:
return 42

inner_task = self.new_task(loop, inner())

await asyncio.wait_for(inner_task, timeout=_EPSILON)

with self.assertRaises(asyncio.TimeoutError):
loop.run_until_complete(foo())

def test_wait_for_self_cancellation(self):
loop = asyncio.new_event_loop()
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
Original file line number Diff line number Diff line change
Expand Up @@ -1592,6 +1592,7 @@ J. Sipprell
Ngalim Siregar
Kragen Sitaker
Kaartic Sivaraam
Roman Skurikhin
Ville Skyttä
Michael Sloan
Nick Sloan
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
When cancelling a task due to timeout, :meth:`asyncio.wait_for` will now
propagate the exception if an error happens during cancellation.
Patch by Roman Skurikhin.