Skip to content

bpo-42130: Fix swallowing of cancellation by wait_for#26097

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.

Sign up for GitHub

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

Closed
ods wants to merge 3 commits into from
Closed

bpo-42130: Fix swallowing of cancellation by wait_for #26097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
acb1522
bpo-42130: Add test for swallowed cancellation
ods May 12, 2021
053f426
Revert GH-21894 as not valid solution for bpo-37658
ods May 13, 2021
fd4ea70
📜🤖 Added by blurb_it.
blurb-it[bot] May 13, 2021
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
15 changes: 6 additions & 9 deletions Lib/asyncio/tasks.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -433,15 +433,12 @@ async def wait_for(fut, timeout):
try:
await waiter
except exceptions.CancelledError:
if fut.done():
return fut.result()
else:
fut.remove_done_callback(cb)
# We must ensure that the task is not running
# after wait_for() returns.
# See https://bugs.python.org/issue32751
await _cancel_and_wait(fut, loop=loop)
raise
fut.remove_done_callback(cb)
# We must ensure that the task is not running
# after wait_for() returns.
# See https://bugs.python.org/issue32751
await _cancel_and_wait(fut, loop=loop)
raise
Comment on lines +436 to +441

@robsdeduderobsdedudeJul 20, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that the state wait_for is in right now (outside this PR) is as bad as
in swallowing a KeyboardInterrupt in a utility function. It makes it unusable
in my opinion. There is also no way for user code to counteract, whereas the
resource leakage could actually be worked around.

This leads me to possibility 1 how I think this issue could be solved:

Merge this PR, embrace the quirk and document it well.
Tell the user something along the lines of

Cancelling wait_for will leave the waited for future/coroutine in any
state between (including both) not started and finished. Make sure to
provide any necessary clean-up on cancellation.

try:
awaitasyncio.wait_for(foo, 123)
exceptasyncio.CancelledError:
... # do clean-upraise

Or alternatively:

try:
future=asyncio.ensure_future(foo) awaitasyncio.wait_for(future, 123)
exceptasyncio.CancelledError:
iffut.done() andnotfut.cancelled() andfut.exception() isNone:
... # the future completed, yet, we got cancelledraise

After all, if the wrapped future yields multiple times to the event loop, you don't get any guarantees as to how far your inner future got. So the user has to be prepared to clean-up the cancellation mess one or the other way.

Possibility 2 I see is to not dismiss, but to defer the cancellation:

Suggested change
fut.remove_done_callback(cb)
# We must ensure that the task is not running
# after wait_for() returns.
# See https://bugs.python.org/issue32751
await_cancel_and_wait(fut, loop=loop)
raise
iffut.done():
# We got cancelled, but we are already done. Therefore, we defer the
# cancellation until next time the task yields to the event loop.
current_task().cancel()
returnfut.result()
else:
fut.remove_done_callback(cb)
# We must ensure that the task is not running
# after wait_for() returns.
# See https://bugs.python.org/issue32751
await_cancel_and_wait(fut, loop=loop)
raise

@robsdeduderobsdedudeJul 20, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To amend to that: this is the issue that I thinks needs a quick resolution (using pytest here, but it should get the point across):

@pytest.mark.asyncioasyncdeftest_wait_for_cancellation_propagates():
inner=asyncio.get_event_loop().create_future()
outer=asyncio.wait_for(inner, 0.1)
outer_future=asyncio.ensure_future(outer)
awaitasyncio.sleep(0)
inner.set_result(None) # inner is doneouter_future.cancel() # AND outer got cancelled# this fails starting with Python 3.8withpytest.raises(asyncio.CancelledError):
awaitouter_future


if fut.done():
return fut.result()
Expand Down
33 changes: 33 additions & 0 deletions Lib/test/test_asyncio/test_asyncio_waitfor.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@ async def run(self):

self.exited = True


class AsyncioWaitForTest(unittest.TestCase):

async def atest_asyncio_wait_for_cancelled(self):
Expand DownExpand Up@@ -56,6 +57,38 @@ async def atest_asyncio_wait_for_timeout(self):
def test_asyncio_wait_for_timeout(self):
asyncio.run(self.atest_asyncio_wait_for_timeout())

async def atest_asyncio_wait_for_hang(self, inner_steps, outer_steps):
# bpo-42130: wait_for can swallow cancellation causing task to hang
async def inner(steps):
for _ in range(steps):
await asyncio.sleep(0)

finished = False

async def wait_for_coro(steps):
nonlocal finished
await asyncio.wait_for(inner(steps), timeout=1)
await asyncio.sleep(0.1)
finished = True

task = asyncio.create_task(wait_for_coro(inner_steps))
for _ in range(outer_steps):
await asyncio.sleep(0)
assert not task.done()

task.cancel()
with self.assertRaises(asyncio.CancelledError):
await task
self.assertFalse(finished)

def test_asyncio_wait_for_hang(self):
# Test with different number of inner/outer steps to weaken dependence
# on implementation details
for inner_steps in range(3):
for outer_steps in range(3):
with self.subTest(inner_steps=inner_steps, outer_steps=outer_steps):
asyncio.run(self.atest_asyncio_wait_for_hang(inner_steps, outer_steps))


if __name__ == '__main__':
unittest.main()
16 changes: 0 additions & 16 deletions Lib/test/test_asyncio/test_tasks.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1147,22 +1147,6 @@ def gen():
res = loop.run_until_complete(task)
self.assertEqual(res, "ok")

def test_wait_for_cancellation_race_condition(self):

@aaliddellaaliddellMay 13, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does this test now fail if it wasn't removed?

@odsodsMay 13, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Exactly. The test is incorrect (the task must raise) and it doesn't reproduce original problem.

def gen():
yield 0.1
yield 0.1
yield 0.1
yield 0.1

loop = self.new_test_loop(gen)

fut = self.new_future(loop)
loop.call_later(0.1, fut.set_result, "ok")
task = loop.create_task(asyncio.wait_for(fut, timeout=1))
loop.call_later(0.1, task.cancel)
res = loop.run_until_complete(task)
self.assertEqual(res, "ok")

def test_wait_for_waits_for_task_cancellation(self):
loop = asyncio.new_event_loop()
self.addCleanup(loop.close)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2021-05-13-11-16-06.bpo-42130.ez0TO5.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
:meth:`asyncio.wait_for` fix swallowing of cancellation