bpo-32751: Wait for task cancellation in asyncio.wait_for() #7216

elprans · 2018-05-29T19:40:56Z

Currently, asyncio.wait_for(fut), upon reaching the timeout deadline,
cancels the future and returns immediately. This is problematic when
fut is a Task, because it will be left running for an arbitrary
amount of time. This behavior is iself surprising and may lead to
related bugs such as the one described in bpo-33638:

condition = asyncio.Condition()
async with condition:
    await asyncio.wait_for(condition.wait(), timeout=0.5)

Currently, instead of raising a TimeoutError, the above code will fail
with RuntimeError: cannot wait on un-acquired lock, because
__aexit__ is reached before condition.wait() finishes its
cancellation and re-acquires the condition lock.

To resolve this, make wait_for await for the task cancellation.
The tradeoff here is that the timeout promise may be broken if the
task decides to handle its cancellation in a slow way. This represents
a behavior change and should probably not be back-patched to 3.6 and
earlier.

https://bugs.python.org/issue32751

Currently, asyncio.wait_for(fut), upon reaching the timeout deadline, cancels the future and returns immediately. This is problematic for when *fut* is a Task, because it will be left running for an arbitrary amount of time. This behavior is iself surprising and may lead to related bugs such as the one described in bpo-33638: condition = asyncio.Condition() async with condition: await asyncio.wait_for(condition.wait(), timeout=0.5) Currently, instead of raising a TimeoutError, the above code will fail with `RuntimeError: cannot wait on un-acquired lock`, because `__aexit__` is reached _before_ `condition.wait()` finishes its cancellation and re-acquires the condition lock. To resolve this, make `wait_for` await for the task cancellation. The tradeoff here is that the `timeout` promise may be broken if the task decides to handle its cancellation in a slow way. This represents a behavior change and should probably not be back-patched to 3.6 and earlier.

1st1 · 2018-05-29T19:45:47Z

Lib/asyncio/tasks.py

+        # We cannot wait on *fut* directly to make
+        # sure _cancel_and_wait itself is reliably cancellable.
+        await waiter
+    except futures.CancelledError:


Do we need this except at all?

No, it's an oversight from an earlier code iteration.

1st1 · 2018-05-29T19:57:32Z

Lib/asyncio/tasks.py

+    waiter = loop.create_future()
+    cb = functools.partial(_release_waiter, waiter)
+    fut.add_done_callback(cb)
+    fut.cancel()


I'd move 'fut.cancel()' into the 'try' block

asvetlov

I suspect if a task has a slow cancellation routine -- many timeout related assumptions will be broken.

LGTM

asvetlov · 2018-05-29T20:42:51Z

Lib/asyncio/tasks.py

+
+    try:
+        fut.cancel()
+        # We cannot wait on *fut* directly to make


Nice trick!

1st1 · 2018-05-29T20:45:13Z

I suspect if a task has a slow cancellation routine -- many timeout related assumptions will be broken.

Yeah. Broken timeouts is a small price to have the rest of asyncio code working correctly. A related bug where wait_for caused an incorrect asyncio.Condition behaviour scared me.

miss-islington · 2018-05-29T21:31:20Z

Thanks @elprans for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…-7216) Currently, asyncio.wait_for(fut), upon reaching the timeout deadline, cancels the future and returns immediately. This is problematic for when *fut* is a Task, because it will be left running for an arbitrary amount of time. This behavior is iself surprising and may lead to related bugs such as the one described in bpo-33638: condition = asyncio.Condition() async with condition: await asyncio.wait_for(condition.wait(), timeout=0.5) Currently, instead of raising a TimeoutError, the above code will fail with `RuntimeError: cannot wait on un-acquired lock`, because `__aexit__` is reached _before_ `condition.wait()` finishes its cancellation and re-acquires the condition lock. To resolve this, make `wait_for` await for the task cancellation. The tradeoff here is that the `timeout` promise may be broken if the task decides to handle its cancellation in a slow way. This represents a behavior change and should probably not be back-patched to 3.6 and earlier. (cherry picked from commit e2b340a) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

bedevere-bot · 2018-05-29T21:32:29Z

GH-7223 is a backport of this pull request to the 3.7 branch.

Currently, asyncio.wait_for(fut), upon reaching the timeout deadline, cancels the future and returns immediately. This is problematic for when *fut* is a Task, because it will be left running for an arbitrary amount of time. This behavior is iself surprising and may lead to related bugs such as the one described in bpo-33638: condition = asyncio.Condition() async with condition: await asyncio.wait_for(condition.wait(), timeout=0.5) Currently, instead of raising a TimeoutError, the above code will fail with `RuntimeError: cannot wait on un-acquired lock`, because `__aexit__` is reached _before_ `condition.wait()` finishes its cancellation and re-acquires the condition lock. To resolve this, make `wait_for` await for the task cancellation. The tradeoff here is that the `timeout` promise may be broken if the task decides to handle its cancellation in a slow way. This represents a behavior change and should probably not be back-patched to 3.6 and earlier. (cherry picked from commit e2b340a) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

@aaliddell

When I was fixing bpo-32751 back in pythonGH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.

@aaliddell

… 0 (#21895) When I was fixing bpo-32751 back in GH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.

@aaliddell

… 0 (pythonGH-21895) When I was fixing bpo-32751 back in pythonGH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency. (cherry picked from commit c517fc7) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

@aaliddell

… 0 (GH-21895) (GH-21963) When I was fixing bpo-32751 back in GH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency. (cherry picked from commit c517fc7) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

@aaliddell

…out <= 0 (pythonGH-21895) When I was fixing bpo-32751 back in pythonGH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.. (cherry picked from commit c517fc7) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

@aaliddell

…out <= 0 (pythonGH-21895) When I was fixing bpo-32751 back in pythonGH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.. (cherry picked from commit c517fc7)

@aaliddell

…out <= 0 (GH-21895) (#21967) When I was fixing bpo-32751 back in GH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.. (cherry picked from commit c517fc7)

@aaliddell

… 0 (python#21895) When I was fixing bpo-32751 back in pythonGH-7216 I missed the case when *timeout* is zero or negative. This takes care of that. Props to @aaliddell for noticing the inconsistency.

elprans requested review from 1st1 and asvetlov as code owners May 29, 2018 19:40

the-knights-who-say-ni added the CLA signed label May 29, 2018

bedevere-bot added the awaiting review label May 29, 2018

1st1 reviewed May 29, 2018

View reviewed changes

elprans added 2 commits May 29, 2018 15:47

Remove redundant cancellation check

9d97213

Move fut.cancel() inside exception block

36fe427

1st1 reviewed May 29, 2018

View reviewed changes

1st1 approved these changes May 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels May 29, 2018

asvetlov approved these changes May 29, 2018

View reviewed changes

Lib/asyncio/tasks.py

try:

fut.cancel()

# We cannot wait on *fut* directly to make

Copy link

Contributor

asvetlov May 29, 2018

Choose a reason for hiding this comment

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

Nice trick!

1st1 merged commit e2b340a into python:master May 29, 2018

bedevere-bot removed the awaiting merge label May 29, 2018

1st1 added the needs backport to 3.7 label May 29, 2018

bedevere-bot removed the needs backport to 3.7 label May 29, 2018

jrs65 mentioned this pull request Oct 9, 2019

Failure acquiring lock chime-experiment/comet#34

Open

elprans mentioned this pull request Aug 15, 2020

Ensure pool connection is released in acquire when cancelled MagicStack/asyncpg#548

Closed

elprans mentioned this pull request Aug 15, 2020

bpo-32751: Wait for task cancel in asyncio.wait_for() when timeout <= 0 #21895

Merged

miss-islington mentioned this pull request Aug 26, 2020

[3.9] bpo-32751: Wait for task cancel in asyncio.wait_for() when timeout <= 0 (GH-21895) #21963

Merged

elprans mentioned this pull request Aug 26, 2020

[3.8] bpo-32751: Wait for task cancel in asyncio.wait_for() when timeout <= 0 (GH-21895) #21967

Merged

elprans mentioned this pull request Aug 26, 2020

[3.7] bpo-32751: Wait for task cancel in asyncio.wait_for() when timeout <= 0 (GH-21895) #21968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32751: Wait for task cancellation in asyncio.wait_for() #7216

bpo-32751: Wait for task cancellation in asyncio.wait_for() #7216

elprans commented May 29, 2018 •

edited

Loading

1st1 May 29, 2018

elprans May 29, 2018

1st1 May 29, 2018

asvetlov left a comment

asvetlov May 29, 2018

1st1 commented May 29, 2018

miss-islington commented May 29, 2018

bedevere-bot commented May 29, 2018

bpo-32751: Wait for task cancellation in asyncio.wait_for() #7216

bpo-32751: Wait for task cancellation in asyncio.wait_for() #7216

Conversation

elprans commented May 29, 2018 • edited Loading

1st1 May 29, 2018

Choose a reason for hiding this comment

elprans May 29, 2018

Choose a reason for hiding this comment

1st1 May 29, 2018

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov May 29, 2018

Choose a reason for hiding this comment

1st1 commented May 29, 2018

miss-islington commented May 29, 2018

bedevere-bot commented May 29, 2018

elprans commented May 29, 2018 •

edited

Loading