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.

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

Jump to bottom

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

Closed
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 number Diff line number Diff 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
Copy link

@robsdedude robsdedude Jul 20, 2022 •
edited
Loading

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:
     await asyncio.wait_for(foo, 123)
 except asyncio.CancelledError:
     ...  # do clean-up
     raise

Or alternatively:

try:
     future = asyncio.ensure_future(foo) 
     await asyncio.wait_for(future, 123)
 except asyncio.CancelledError:
     if fut.done() and not fut.cancelled() and fut.exception() is None:
         ...  # the future completed, yet, we got cancelled
     raise

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
if fut.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()
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

Copy link

@robsdedude robsdedude Jul 20, 2022 •
edited
Loading

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.asyncio
async def test_wait_for_cancellation_propagates():
    inner = asyncio.get_event_loop().create_future()
    outer = asyncio.wait_for(inner, 0.1)
    outer_future = asyncio.ensure_future(outer)
    await asyncio.sleep(0)
    inner.set_result(None)  # inner is done
    outer_future.cancel()  # AND outer got cancelled

    # this fails starting with Python 3.8
    with pytest.raises(asyncio.CancelledError):
        await outer_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 number Diff line number Diff 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 Down Expand 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 number Diff line number Diff 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):
Copy link

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?

Copy link
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 number Diff line number Diff line change
@@ -0,0 +1 @@
:meth:`asyncio.wait_for` fix swallowing of cancellation