Fix AtomicCondition compile error ziglang/zig#8051 #8637
Conversation
This also changes the `wait(...)` signature to take a `Mutex.Impl.Held`
object instead of a plain `Mutex`, which IIUC is clearer to the intent
of the function (ie., you should be passing in a locked mutex, not just
any mutex).
For example:
var mutex = Mutex{};
var cond = Condition{};
var held = mutex.acquire();
defer held.release();
while (bad_state == true) {
// cond.wait(&mutex); // <-- Original
cond.wait(&held); // <-- Updated
}
Not really a breaking change for code using `AtomicCondition` (since it
didn't compile before), but this does affect the other implementations
and would presumably break anything using that code as well.
jedisct1
added
breaking
|
cc @kprotty Don't know if you're still plan on overhauling the whole set of synchronization primitives but I'd like to get this merged. Can you please add some basic tests, preferably not using any |
Awesome news!
The latter, VMs used in CI are slow and many tests where we tried to order the operations on two different threads using |
This also changes the `wait(...)` signature to take a `Mutex.Impl.Held`
object instead of a plain `Mutex`, which IIUC is clearer to the intent
of the function (ie., you should be passing in a locked mutex, not just
any mutex).
For example:
var mutex = Mutex{};
var cond = Condition{};
var held = mutex.acquire();
defer held.release();
while (bad_state == true) {
// cond.wait(&mutex); // <-- Original
cond.wait(&held); // <-- Updated
}
Not really a breaking change for code using `AtomicCondition` (since it
didn't compile before), but this does affect the other implementations
and would presumably break anything using that code as well.
|
@cescude PR which Condition and Semaphore tests |
Add tests to Condition and Semaphore
|
@cescude You're right in that the extra signal() in Semaphore.wait() can be removed, although i'm not sure how
|
|
Ah, ok, I think you're right.
Nevermind, I'm not thinking clearly here—permits will increase which makes the first wait go through. |
|
I'd like to get rid of the |
|
@andrewrk Curious as to what you're thinking wrt how this negatively affects composability-- When I think about "which operations are valid" on a locked vs unlocked mutex...there's no overlap (so it makes sense to me to have distinct types exposed for an API). In getting rid of |
|
Releasing a mutex without having an acquired handle is common for code which is split by asynchrony (e.g. lock in one function, unlock in another). Also, if you wish to acquire after releasing it, you'd need a reference to the initial Mutex. Having Held and *Mutex both be the same thing underneath feels awkward compared to *Mutex having release() along with acquire(). |
🤔 Should it be common, though? IIUC splitting by asynchrony could only apply to a single-threaded async implementation; using a multi-threaded event loop and you might get
Maybe; it's not like we have a different type for open vs closed files, so I get that |
Zig's stdlib Mutex doesn't (and shouldn't in the near future) have to be based on the OS' mutex implementation. Thread-bound critical section calls is mostly used internally for fair scheduling or priority inheritance, things our Mutex type currently don't guarantee.
You can bind an asynchronous task to a thread in a multi-threaded event loop. But irrespective that part, the "asynchrony" here doesn't imply Zig's
It does for simple, linear-lifetime, RAII-like use cases but becomes cumbersome otherwise IMO. The unlocking/locking scenario requires storing two pointers to the same data structure for example. A popular Rust mutex impl also has a |
FWIW, it's interesting that the Anyways, thanks for taking the time to discuss. |
|
Np, this feels like it should be an issue so that more people could weight in on their preferences if they have any. |
|
Closing old draft |
This also changes the
wait(...)signature to take aMutex.Impl.Heldobject instead of a plain
Mutex, which IIUC is clearer to the intentof the function (ie., you should be passing in a locked mutex, not just
any mutex).
For example:
Not really a breaking change for code using
AtomicCondition(since itdidn't compile before), but this does affect the other implementations
and would presumably break anything using that code as well.