classification
Title: Asyncio.Condition prevents cancellation
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, bar.harel, njs, yselivanov
Priority: normal Keywords: patch

Created on 2018-02-13 21:30 by bar.harel, last changed 2018-05-25 20:17 by bar.harel. This issue is now closed.

Files
File name Uploaded Description Edit
le_bug.py bar.harel, 2018-02-13 21:30
le_patch.diff bar.harel, 2018-02-13 21:30
Pull Requests
URL Status Linked Edit
PR 5665 merged bar.harel, 2018-02-13 22:25
PR 5682 merged miss-islington, 2018-02-14 09:19
PR 5683 merged miss-islington, 2018-02-14 09:20
Messages (13)
msg312140 - (view) Author: Bar Harel (bar.harel) * Date: 2018-02-13 21:30
Hey guys,

A week after a serious asyncio.Lock bug, I found another bug that makes asyncio.Condition ignore and prevent cancellation in some cases due to an "except: pass" which tbh is a little embarrassing.

What happens is that during the time it takes to get back a conditional lock after notifying it, asyncio completely ignores all cancellations sent to the waiting task.

le_bug.py: Contains the bug
le_patch.diff: Contains a very simple fix (will send a pull on Github too)
le_meme.jpg: Contains my face after debugging this for 4 hours

Yuri, I hope you didn't miss me during this week ;-)

-- Bar
msg312141 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-02-13 21:59
le_meme.jpg is a pretty common facial expression when one debugs locks/conditions, so let's not attach it in the future :)  We try to keep the bug tracker free of unnecessary information.

As for the bug—please submit a PR!
msg312143 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-02-13 23:00
Interesting case! This is super subtle. I think the patch is correct though.

(In Trio this is one of the cases where we use shielding: https://github.com/python-trio/trio/blob/f48d8922dfe2118bfaaf9a85f2524e58146ba369/trio/_sync.py#L749-L752 . But asyncio.shield has different semantics -- it still delivers the CancelledError to the caller, which is no good in this case.)
msg312157 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-02-14 05:28
It does make me wonder if asyncio.shield *should* wait for the thing it's shielding though, so that it *would* work in this case? (Similar to bpo-32751.)
msg312163 - (view) Author: Bar Harel (bar.harel) * Date: 2018-02-14 08:12
I don't think so. Having shield not cancel immediately but rather wait and
cancel will cause long timed shielded operations to stall the task
cancellation, usually for no good. This isn't the general case.
However, adding another function which does so might just be a good idea. I
think another parameter to shield to choose cancellation time will clutter
the function call.

On Wed, Feb 14, 2018, 7:28 AM Nathaniel Smith <report@bugs.python.org>
wrote:

>
> Nathaniel Smith <njs@pobox.com> added the comment:
>
> It does make me wonder if asyncio.shield *should* wait for the thing it's
> shielding though, so that it *would* work in this case? (Similar to
> bpo-32751.)
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue32841>
> _______________________________________
>
msg312164 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-02-14 09:18
New changeset 5746510b7aef423fa4afc92b2abb919307b1dbb9 by Andrew Svetlov (Bar Harel) in branch 'master':
bpo-32841: Fix cancellation in awaiting asyncio.Condition (#5665)
https://github.com/python/cpython/commit/5746510b7aef423fa4afc92b2abb919307b1dbb9
msg312165 - (view) Author: miss-islington (miss-islington) Date: 2018-02-14 09:47
New changeset 8caee0fa572e8ced00df553a7bdca49ddaf729e8 by Miss Islington (bot) in branch '3.7':
bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665)
https://github.com/python/cpython/commit/8caee0fa572e8ced00df553a7bdca49ddaf729e8
msg312166 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-02-14 10:10
New changeset a23eecab9a0b724bdfde83d159ac2415927f042a by Andrew Svetlov (Miss Islington (bot)) in branch '3.6':
bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665) (GH-5683)
https://github.com/python/cpython/commit/a23eecab9a0b724bdfde83d159ac2415927f042a
msg312167 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-02-14 10:11
Thanks Bar Harel
msg312168 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-02-14 10:21
> Having shield not cancel immediately but rather wait and cancel will cause long timed shielded operations to stall the task cancellation, usually for no good. This isn't the general case.

What I'm suggesting is that maybe it actually is good in the general case :-). If an operation can't be cancelled, that's too bad, but it's generally still better to wait then to silently leak it into the background.
msg312169 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-02-14 10:24
Well, makes sense
msg317702 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-05-25 19:35
Should this issue be closed now?
msg317715 - (view) Author: Bar Harel (bar.harel) * Date: 2018-05-25 20:17
Yup. Thanks Yuri :-)
History
Date User Action Args
2018-05-25 20:17:45bar.harelsetstatus: open -> closed
resolution: fixed
messages: + msg317715

stage: patch review -> resolved
2018-05-25 19:35:42yselivanovsetmessages: + msg317702
2018-02-14 10:24:25asvetlovsetmessages: + msg312169
2018-02-14 10:21:20njssetstatus: closed -> open

nosy: - miss-islington
messages: + msg312168

resolution: fixed -> (no value)
stage: resolved -> patch review
2018-02-14 10:11:19asvetlovsetstatus: open -> closed
resolution: fixed
messages: + msg312167

stage: patch review -> resolved
2018-02-14 10:10:25asvetlovsetmessages: + msg312166
2018-02-14 09:47:38miss-islingtonsetnosy: + miss-islington
messages: + msg312165
2018-02-14 09:20:20miss-islingtonsetpull_requests: + pull_request5478
2018-02-14 09:19:28miss-islingtonsetpull_requests: + pull_request5477
2018-02-14 09:18:19asvetlovsetmessages: + msg312164
2018-02-14 08:12:46bar.harelsetmessages: + msg312163
2018-02-14 05:28:19njssetmessages: + msg312157
2018-02-13 23:00:51njssetnosy: + njs
messages: + msg312143
2018-02-13 22:25:26bar.harelsetstage: patch review
pull_requests: + pull_request5467
2018-02-13 21:59:14yselivanovsetfiles: - le_meme.jpg
2018-02-13 21:59:08yselivanovsetmessages: + msg312141
2018-02-13 21:30:18bar.harelsetfiles: + le_meme.jpg
2018-02-13 21:30:09bar.harelsetfiles: + le_patch.diff
keywords: + patch
2018-02-13 21:30:02bar.harelcreate