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

gh-102780: Fix uncancel() call in asyncio timeouts #102815

Merged
merged 13 commits into from
Mar 22, 2023
16 changes: 13 additions & 3 deletions Doc/library/asyncio-task.rst
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,17 @@ in the task at the next opportunity.
It is recommended that coroutines use ``try/finally`` blocks to robustly
perform clean-up logic. In case :exc:`asyncio.CancelledError`
is explicitly caught, it should generally be propagated when
clean-up is complete. Most code can safely ignore :exc:`asyncio.CancelledError`.
clean-up is complete. :exc:`asyncio.CancelledError` directly subclasses
:exc:`BaseException` so most code will not need to be aware of it.

The asyncio components that enable structured concurrency, like
:class:`asyncio.TaskGroup` and :func:`asyncio.timeout`,
gvanrossum marked this conversation as resolved.
Show resolved Hide resolved
are implemented using cancellation internally and might misbehave if
a coroutine swallows :exc:`asyncio.CancelledError`. Similarly, user code
should not call :meth:`uncancel <asyncio.Task.uncancel>`.
should not generally call :meth:`uncancel <asyncio.Task.uncancel>`.
However, in cases when suppressing :exc:`asyncio.CancelledError` is
truly desired, it is necessary to also call ``uncancel()`` to completely
remove the cancellation state.

.. _taskgroups:

Expand Down Expand Up @@ -1148,7 +1152,9 @@ Task Object
Therefore, unlike :meth:`Future.cancel`, :meth:`Task.cancel` does
not guarantee that the Task will be cancelled, although
suppressing cancellation completely is not common and is actively
discouraged.
discouraged. Should the coroutine nevertheless decide to suppress
the cancellation, it needs to call :meth:`Task.uncancel` in addition
to catching the exception.

.. versionchanged:: 3.9
Added the *msg* parameter.
Expand Down Expand Up @@ -1238,6 +1244,10 @@ Task Object
with :meth:`uncancel`. :class:`TaskGroup` context managers use
:func:`uncancel` in a similar fashion.

If end-user code is, for some reason, suppresing cancellation by
catching :exc:`CancelledError`, it needs to call this method to remove
the cancellation state.

.. method:: cancelling()

Return the number of pending cancellation requests to this Task, i.e.,
Expand Down
7 changes: 4 additions & 3 deletions Lib/asyncio/timeouts.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ def __repr__(self) -> str:
async def __aenter__(self) -> "Timeout":
self._state = _State.ENTERED
self._task = tasks.current_task()
self._cancelling = self._task.cancelling()
if self._task is None:
raise RuntimeError("Timeout should be used inside a task")
self.reschedule(self._when)
Expand All @@ -104,10 +105,10 @@ async def __aexit__(
if self._state is _State.EXPIRING:
self._state = _State.EXPIRED

if self._task.uncancel() == 0 and exc_type is exceptions.CancelledError:
# Since there are no outstanding cancel requests, we're
if self._task.uncancel() <= self._cancelling and exc_type is exceptions.CancelledError:
# Since there are no new cancel requests, we're
# handling this.
raise TimeoutError
raise TimeoutError from exc_val
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)

Copy link
Contributor

@kumaraditya303 kumaraditya303 Mar 19, 2023

Choose a reason for hiding this comment

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

Isn't this done by default? I see the same traceback with and without this on this example:

import asyncio

async def func():
    await asyncio.sleep(1)

async def main():
    async with asyncio.timeout(0.1):
        await func()

asyncio.run(main())

Copy link
Member

Choose a reason for hiding this comment

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

Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.__cause__, while without it, it gets stored in exc.__context__, and the connecting phrase in the output is different:

__cause__:

The above exception was the direct cause of the following exception:

__context__:

During handling of the above exception, another exception occurred:

(If both are set it follows __cause__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Precicely. I has bothered me for a while that timeouts happen with the latter message, as if somehow, there was an error during exception handling. I wanted to use the opportunity to roll that fix in :)

Copy link
Contributor Author

@kristjanvalur kristjanvalur Mar 20, 2023

Choose a reason for hiding this comment

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

This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)

Thank you, agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.cause, while without it, it gets stored in exc.context, and the connecting phrase in the output is different:

Ah, right! I missed that. Adding a test would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that in a few hours.

elif self._state is _State.ENTERED:
self._state = _State.EXITED

Expand Down
30 changes: 30 additions & 0 deletions Lib/test/test_asyncio/test_timeouts.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,36 @@ async def test_nested_timeout_in_finally(self):
async with asyncio.timeout(0.01):
await asyncio.sleep(10)

async def test_timeout_after_cancellation(self):
try:
asyncio.current_task().cancel()
await asyncio.sleep(1) # work which will be cancelled
except asyncio.CancelledError:
pass
finally:
with self.assertRaises(TimeoutError):
async with asyncio.timeout(0.0):
await asyncio.sleep(1) # some cleanup

async def test_cancel_in_timeout_after_cancellation(self):
try:
asyncio.current_task().cancel()
await asyncio.sleep(1) # work which will be cancelled
except asyncio.CancelledError:
pass
finally:
with self.assertRaises(asyncio.CancelledError):
async with asyncio.timeout(1.0):
asyncio.current_task().cancel()
await asyncio.sleep(2) # some cleanup

async def test_timeout_exception_cause (self):
with self.assertRaises(asyncio.TimeoutError) as exc:
async with asyncio.timeout(0):
await asyncio.sleep(1)
cause = exc.exception.__cause__
assert isinstance(cause, asyncio.CancelledError)


if __name__ == '__main__':
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
The :class:`asyncio.Timeout` context manager now works reliably even when performing cleanup due
to task cancellation. Previously it could raise a
:exc:`~asyncio.CancelledError` instead of an :exc:`~asyncio.TimeoutError` in such cases.