-
-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move trio.Condition into trio.hazmat, or possibly remove altogether? #1082
Comments
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
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 (@njsmith, I suspect you know all this, but am adding it to the issue in case it's useful context for others. :-) ) |
Right, a
Huh, that's a somewhat subtle and interesting side-effect of how our fair mutexes work. For those following along: ...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 |
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 Yes I know this is not a particularly strong argument. So sue me. :-P |
I remembered there actually is one classic use of a condition-variable like pattern in trio's sources, in trio/trio/testing/_memory_streams.py Lines 463 to 475 in 07af2f8
The reason this doesn't use an actual |
NB: the pattern in the above post is basically the same as the stdlib's
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. |
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 wantedtrio.Condition
. They have weird semantics (there are only four uses of cancel shielding inside Trio, and one of them is intrio.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 movingCondition
intotrio.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 handleCondition
. (Specifically the stuff about fair requeueing betweenParkingLot
s.) We don't even useParkingLot
nearly as much as I expected we would. It does need some tight integration withtrio.Lock
, so if you wanted to replicate it in a third-party library you'd probably need your ownLock
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.The text was updated successfully, but these errors were encountered: