Thanks for this @jamshark70 , sorry it took a while to review and sorry I have g…enerally
not been participating that much in these discussions, I know it is a major pain point for
you with testing.
My opinion after taking some time to think about this and reviewing condition variables in
other libraries is that Condition in SC has too many defects to be workable, and that we
should instead put effort into designing a new object with a simpler and easier to use
interface (which I will tentatively call `CondVar` to avoid confusion). what I'm
suggesting combines ideas from both this PR and from #3895, would probably go through the
RFC process, and most likely target the 3.13.0 release, although it could be done in time
for 3.12.0.
Also for clarity's sake, below I'm going to refer to the typically Function-like object
that's held by Condition:-test as the **predicate** rather than **test** because the
latter has overloaded meaning.
I'm going to share what I see as the major flaws of Condition, and then what I'd suggest
as the new interface to replace it.
#### 1. The predicate is given to the constructor and is held by the class
Declaring the predicate when constructing the Condition variable is inconvenient. Also,
having just one predicate which is managed by the class is an unnecessary restriction and
introduces state where none is necessary. Instead, the predicate should be passed to
`wait` itself as an optional parameter; or, if desired, there can be separate methods for
waiting and for waiting-with-predicate, a bit like how `hang` and `wait` are right now.
Personally, I would combine them to a single function, and this is what I suggested below.
Some issues with a Condition with stateful predicate:
- it's confusing that Condition takes a `test` parameter in its constructor, but that it
isn't always used (in hang and unhang, for example).
- it cannot support the cases where multiple waiting threads want to consume the same
resource in differing ways or degrees, and therefore have different predicates.
- everything can be managed without passing any predicates to Condition at all, and in
fact this is what many Condition designs do.
- the predicate may depend on variables/state available long after the Condition has been
constructed, requiring users to make an unnecessary choice between rearranging code to
fit this requirement, or setting `cond.test` later on, which may lead to confusing bugs
regarding state manipulation.
- it unnecessarily separates the predicate code from the callsite of `wait`, when we
usually care about both together.
- the predicate is not guaranteed to always be evaluated on the same thread.
For comparison, here is the same bit of code using Condition, and then a hypothetical
CondVar which supports wait-with-predicate:
<details>
<summary>Code</summary>
```
(
var x = 0;
var c = Condition { x.postln == 6 }; // must be declared after x!
/* lots of code */
// have to jump up to see the predicate; no guarantee on what thread the predicate is
// evaluated
fork { c.wait; "done".postln; };
fork { loop { x = x + 1; c.signal; 0.25.wait; } };
)
(
var c = Condition(); // predicate set below
var x = 0; // can now be declared after `c`
/* lots of code */
// set predicate as close as possible to `wait` callsite; risky because we may be
// overwriting `c.test` without knowing it, and because some later code may overwrite it.
c.test = { x.postln == 6 };
fork { c.wait; "done".postln; };
fork { loop { x = x + 1; c.signal; 0.25.wait; } };
)
(
var c = CondVar(); // can declare before or after `x`
var x = 0;
/* lots of code */
// predicate is immediately clear, and always evaluated on the thread calling `wait`
fork { c.wait { x.postln == 6 }; "I have 6".postln };
// we can even have multiple waits with distinct predicates
fork { c.wait { x.postln == 5 }; "I have 5".postln };
fork { loop { x = x + 1; c.signal; 0.25.wait; } };
)
```
</details>
#### 2. The hang/wait and signal/unhang distinctions break the abstraction of the class
Condition not only supports two orthogonal interfaces at once, but it also allows them to
be mixed with bad results. `unhang` can wake `wait`ing threads even when `test.value` is
`false`, and `signal` will only wake `hang`ing threads if `test.value` is `true`. This
behavior is undesirable because there is no way to tell after a call to `wait` returns
whether or not the predicate is true (or tested true when the thread was woken). It is
also confusing for users to learn the difference between these two interfaces, when things
can actually be much simpler and more consistent with a single interface. Instead of both
`signal` and `unhang`, there should be a single method, `signal` (or `notify`, typically
used for other implementations) which wakes threads, and then the predicate test is run on
each thread independently and the decision to continue blocking is made there. Instead of
both `wait` and `hang`, there should be a single method `wait`, which has a single
optional predicate parameter. With this interface, it is possible to guarantee that
a thread returns from `wait` if and only if the condition was signaled and the predicate,
if one was passed, is true.
For comparison:
<details>
<summary>Code</summary>
```
(
c = Condition(); // `test` defaults to `false`
// first thread will never be woken, as by default `c.test === false`
fork { c.hang; "hang done".postln; };
fork { 1.wait; c.signal; };
// first thread will be woken, even though `c.test === false`
fork { c.wait; "wait done".postln; };
fork { 1.wait; c.unhang; };
// looks correct, but misuses interface -- `unhang` does not depend on `cond.test.value`
fork { c.wait; "second wait done".postln; };
fork { 1.wait; c.test = true; c.unhang; };
// is correct, but strange use of interface -- return from `hang` deceptively depends on `cond.test.value`
fork { c.hang; "second wait done".postln; };
fork { 1.wait; c.test = true; c.signal; };
)
```
</details>
#### 3. There is no easy way to wake up a single waiting/hanging thread
All implementations of Condition I could find support waking a single thread as opposed to
waking all threads. This is desirable whenever we have multiple threads waiting on a
single condition and we know only one will be able to use the resource guarded by the
condition next. So, CondVar should support both `notifyOne` and `notifyAll`. The verb
`signal` can also be used instead of `notify`; this is just the most common name I see for
the operation.
#### Proposed new interface
<details>
<summary>Here is the interface I think we should approach for CondVar.</summary>
```
/* Creates a CondVar.
*/
new()
/* If `pred` is `nil` (its default value), blocks the current thread until woken by
* `notifyOne()` or `notifyAll()`. Otherwise, if `pred.value` returns true, returns
* immediately. Otherwise, blocks the current thread until woken by `notifyOne()` or
* `notifyAll()` and `pred.value` returns `true`. `pred` is typically a Boolean or Function.
* If `pred` throws an exception, the exception will be propagated up on the thread which
* called `wait`, and the thread which called it will no longer be considered waiting on
* this CondVar.
*
* With a predicate, this method is equivalent to:
*
* while { pred.value.not } { cond.wait }
*
* Returns this object.
*/
wait(pred)
/* Wakes one thread waiting on this Condition. Threads are woken in the order in which
* they called `wait()`. If a thread is woken and was waiting with a predicate, and that
* predicate is still `false`, it will `wait` again and be placed at the end of the queue
* of threads waiting on this CondVar. Another thread will not be woken in that case.
* Returns this object.
*/
notifyOne()
/* Similar to `notifyOne()` but all threads waiting on this CondVar are woken.
* Returns this object.
notifyAll()
/* If `pred` is not nil and `pred.value` returns true, returns immediately. Otherwise,
* blocks the current thread until either the thread is woken by `notifyOne()` or
* `notifyAll()` and `pred.value` returns `true`, or `beats` time in clock beats has
* elapsed. Returns `false` if the timeout elapsed and either `pred` was `nil` or
* `pred.value` was `false`, otherwise `true`. In other words, this method behaves much like
* `wait` except that if it returns `false`, the wait timed out without a passed predicate
* becoming true.
* If `pred` throws an exception, the exception will be propagated up on the thread which
* called `waitWithTimeout`, and the thread which called it will no longer be considered
* waiting on this CondVar.
*
* With a predicate, this is equivalent to:
*
* var beatsRemaining = beats;
* while { pred.value.not } {
* if(cond.waitWithTimeout(beatsRemaining).not) {
* ^pred.value
* };
* // Very generalized pseudocode -- necessary because this is
* // wait-for-duration and not wait-until-timepoint.
* beatsRemaining = calculateBeatsRemaining(thisThread.threadPlayer, beats);
* }
* ^true
*
* If `beats` is not a Float or Integer, an exception is immediately thrown.
*/
waitWithTimeout(beats, pred)
/* Throws an error; CondVar is not shallow-copyable or copyable.
*/
shallowCopy()
/* Throws an error; CondVar is not deep-copyable.
*/
deepCopy()
```
</details>
#### Exception safety
I also haven't used an `action` callback like in your `timeoutAfter` design; this is
because I think the same thing can be achieved with greater simplicity by returning a
Boolean indicating whether the timeout expired without an optional predicate becoming
true. Adding a timeout-expired callback requires us to think about (1) what happens if an
exception is thrown in that callback, and (2) which thread the callback should run on.
For comparison:
<details>
<summary>Code</summary>
```
(
c = Condition();
// To what does `thisThread` refer? How do we catch the exception?
c.waitWithTimeout(3) { thisThread.hash.postln; Exception("whoops!").throw };
// `thisThread` refers to the thread calling `waitWithTimeout`; the exception will
// propagate on this thread's stack as usual.
if(c.waitWithTimeout(3).not) { thisThread.hash.postln; Exception("whoops!").throw };
)
```
</details>
I'm also intentionally suggesting that an error be thrown on the calling thread if beats
isn't a Float or Integer, because it's much simpler to manage an exception being thrown
that way than deep within a forked thread. I'm not sure if the implementation will allow
this but ideally it should be almost impossible for an exception to be thrown while
"context switching", because this just creates massive headaches for anyone trying to
understand the code or debug an error they hit.
---
Initially, I was thinking it might be possible to get to this interface through a series
of deprecations. However, this would be really quite tricky to do without any mistakes,
and if we are going to start recommending Condition to more people, my view is that it
would be better to not burden them with documentation that has to explain both the old and
new interfaces. I would rather just deprecate the entire class and start with a fresh one.
Sorry if this seems like a lot; I know you were kind of expecting this to be merged as-is,
but I really don't think this is a good idea. For such a fundamental class, I really think
think we need to get the abstraction right, and right now I just feel there are too many
issues with Condition as it's currently implemented.
Let me know how you'd like to proceed. If you think this looks all right, maybe we could
proceed with a new PR. However, I feel like it may be better to go to an RFC, which I'd be
happy to initiate if you don't feel you have enough time to manage it. Also, if this is
going to take a while, we should actually probably add a quick band-aid so that these
failing tests work again.