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

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!

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.)

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.)

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>
> _______________________________________
>

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

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

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

Thanks Bar Harel

> 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.

Well, makes sense

Should this issue be closed now?

Yup. Thanks Yuri :-)

stage: patch review -> resolved

nosy: - miss-islington
messages: + msg312168

resolution: fixed -> (no value)
stage: resolved -> patch review

stage: patch review -> resolved