gh-112202: Ensure that condition.notify() succeeds even when racing with Task.cancel()

[kristjanvalur]

If Condition.notify() raises an exception that could have occurred after the task was selected to be woken up, wake up another task instead, if available.

This ensures that the typical case, of waking up one task to handle something, will succeed even if the candidate task
is cancelled at the same time or otherwise wakes up with an error.

This may result in a spurious wakeup of a waiting task, which is what the Condition Variable protocol specifies, precisely because waking up tasks conservatively (rather wake up too many than too few) is the safest and simplest to implement, and missing wakeups are serious (and cause deadlocks), while extra wakeups are easily handled by application code.

IMHO trying to guarantee that we never perform any extra wakeups would complicate the code too much and be too hard to verify, particularly since that is not a guarantee that Condition Variables are supposed to provide.

The PR includes

• Tests
• Fix to asyncio.Condition where raising an exception out of condition.wait after having released the lock will result in a second task being awoken, if available
• Documentation changes explaining the Condition Variable protocol and how if favors too many wakeups over to few wakeups (i.e. how "spurious wakeups" may occur)

---

📚 Documentation preview 📚: https://cpython-previews--112201.org.readthedocs.build/

• Issue: Race between asyncio Condition.notify() and Task.cancel() may result in lost wakeups. #112202

[gvanrossum]

I'm reviewing this, but (as usual with locking changes) it is hard work (thanks for helping out here!). Plus I need clarity about the relationship with gh-111694.

[gvanrossum]

Turned it into a draft until gh-111694 is merged.

[gvanrossum]

I've put off reviewing this PR until gh-111694 lands, but I had already started some doc nits, so here they are. Also note I changed this to Draft mode -- please undraft it once the other PR is merged (into main, and here).

[gvanrossum]

Note that the docstring for notify_all() below mentions threads (twice). That should probably be changed to tasks. (Or coroutines, like above? Though IMO that should also be tasks -- in practice all coroutines are wrapped by tasks, and tasks are the unit of control that users are encouraged to think in terms of.)

[kristjanvalur]

Yes, I think the mention of "coroutines" is a relic from the very old days.
The locks.py discusses coroutines in a lot of the docstrings where "tasks" are more appropriate. scheduling works on Task objects, not coroutines. I can change it wholesale, do I use "Task" or "task" when doing so?

[gvanrossum]

I'd use "task" -- it doesn't really matter whether they are technically instances of asyncio.Task, and in fact IIRC even loops may override create_task() to return instances of some other class.

[kristjanvalur]

Thanks. I take it you approve of me going over the other inline docs/comments and making that correction, I'll do that in a separate commit.

[gvanrossum]

Same issue as in the docs above.

[kristjanvalur]

I've rebased and re-opened this pr, will address the comments presently.

[gvanrossum]

Thanks, we're close to merging this now.

I wonder if we should use "awakened" consistently instead of "awoken"?

PS. I will admit to not carefully reading through the tests, I trust you have got this right.

[gvanrossum]

I'd use "task" -- it doesn't really matter whether they are technically instances of asyncio.Task, and in fact IIRC even loops may override create_task() to return instances of some other class.

[kristjanvalur]

Thanks, we're close to merging this now.

I wonder if we should use "awakened" consistently instead of "awoken"?

I think you are right, I fixed it.

PS. I will admit to not carefully reading through the tests, I trust you have got this right.

I certainly hope so :). I made sure they failed without the fix, it's easy to test.

[gvanrossum]

Two doc nits, then I'll merge.

[kristjanvalur]

Thank you for your help, Guido!