Move trio.Condition into trio.hazmat, or possibly remove altogether?

[njsmith]

I was reading @bcmills' slides Rethinking Classical Concurrency Patterns (linked from here), and among other things it has a bunch of interesting discussion of condition variables and how tricky they are to use (starting on slide 37). It also led me to golang/go#21165, where he proposed deprecating Go's Cond type entirely.

It's true, they really are stupidly hard to use correctly. The main arguments for them are (a) they're a very efficient low-level primitive that's often used to build higher-level tools, (b) they're extremely traditional and taught in every textbook, so people expect to have them.

In Trio, though, they really aren't very attractive. They're not particularly efficient, and we don't use them as a primitive to implement anything else – when you want a fast low-level finicky thing, you use wait_task_rescheduled instead. I've never found a situation where I wanted trio.Condition. They have weird semantics (there are only four uses of cancel shielding inside Trio, and one of them is in trio.Condition, and it's the only case that can create an uncancellable deadlock). And even if we do provide them, there's no particular reason they need to be in the top-level namespace. So I think the argument for at least moving Condition into trio.hazmat is pretty clear.

Should it exist at all? It's definitely a bit annoying to implement – there's a chunk of complexity in trio.hazmat.ParkingLot that's exclusively to handle Condition. (Specifically the stuff about fair requeueing between ParkingLots.) We don't even use ParkingLot nearly as much as I expected we would. It does need some tight integration with trio.Lock, so if you wanted to replicate it in a third-party library you'd probably need your own Lock implementation as well. (Or maybe this is only if you want to be finicky about fairness? I forget. Should figure this out.)

#637, the issue about deprecating Event.clear, is also closely related.

[oremanj]

I think of condition variables as the thing to reach for when you want to wait until a certain condition is true. But usually, checking the condition doesn't require performing any async operations, so thanks to cooperative multitasking you can just say

while not <condition>:
    await self._lot.park()

which is a lot less error-prone. If we do deprecate or hazmatify Condition, I think the docs should mention the above pattern.

The main residual value of Condition is that it provides fair multi-wakeup. Using unpark_all() in the above example will wake up all waiters, but they get scheduled in some unspecified order. Using Condition.notify_all() will wake them up in the order they went to sleep.

(@njsmith, I suspect you know all this, but am adding it to the issue in case it's useful context for others. :-) )

[njsmith]

Right, a Condition is exactly for waiting for some mutex-protected state to satisfy some condition. And yeah, in Trio "mutex-protected state" is not super common, both because of the implicit locking that we get from the GIL/await, and because we have nice friendly alternatives like channels. But I guess if we provide Lock and support mutex-protected state at all then we should assume that sometimes people will need to wait for it to satisfy a condition...

The main residual value of Condition is that it provides fair multi-wakeup. Using unpark_all() in the above example will wake up all waiters, but they get scheduled in some unspecified order. Using Condition.notify_all() will wake them up in the order they went to sleep.

Huh, that's a somewhat subtle and interesting side-effect of how our fair mutexes work. For those following along: ParkingLot in general is fair – tasks are woken in the order they went to sleep. But unpark_all() of course wakes all the tasks, so there's no ordering involved. Condition.broadcast also wakes all tasks. But! In order to wake up, they first have to reacquire the Lock, and only one of them can do this at a time, so their wakeups do end up being ordered. And because we jump through some complicated hoops internally, it turns out that the order they acquire the Lock matches the order that they called Condition.wait.

...Then in common examples like the one given:

async with condition:
    while not <predicate>:
        await condition.wait()

...we will often throw that away as they all wake up and then go back to sleep again, not necessarily in the same order. Well, I guess this only happens if evaluating the predicate requires an await, so maybe it's not that common? Hard to say because so far we have zero examples of Condition use in the wild to refer to :-).

[smurfix]

I'd like to keep them, if only because every other libray has them – and it'd be annoying to refactor the code that uses them, when I sprinkle yet another thread-(ab)using library with async/await` in order to to convince it to work with Trio instead.

Yes I know this is not a particularly strong argument. So sue me. :-P

[njsmith]

I remembered there actually is one classic use of a condition-variable like pattern in trio's sources, in _LockstepByteQueue where I gave up on trying to do fine-grained tracking of who-wakes-up-in-response-to-what:

trio/trio/testing/_memory_streams.py

Lines 463 to 475 in 07af2f8

	def _something_happened(self):
	self._waiters.unpark_all()
	# Always wakes up when one side is closed, because everyone always reacts
	# to that.
	async def _wait_for(self, fn):
	while True:
	if fn():
	break
	if self._sender_closed or self._receiver_closed:
	break
	await self._waiters.park()
	await _core.checkpoint()

The reason this doesn't use an actual Condition is that we effectively take care of the locking through ConflictDetector instead of Lock. The locking is actually really subtle though, because we have two different "locks" used by different operations and two operations that don't take any locks at all. It would probably be easier to understand if it did use a Lock + Condition...

[njsmith]

NB: the pattern in the above post is basically the same as the stdlib's threading.Condition.wait_for. We don't have a wait_for method on trio.Condition currently.

@smurfix

Yes I know this is not a particularly strong argument. So sue me. :-P

Eh, how easy we make it to port stuff to Trio is not the only consideration, but it's certainly not irrelevant either; thanks for pointing that out.