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
Fix for Issue #8051: std.Thread.Condition: error: no member named 'unlock' in struct 'std.Thread.Mutex' #10105
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,7 +106,7 @@ pub const AtomicCondition = struct { | |
.linux => { | ||
switch (linux.getErrno(linux.futex_wait( | ||
&cond.futex, | ||
linux.FUTEX_PRIVATE_FLAG | linux.FUTEX_WAIT, | ||
linux.FUTEX.PRIVATE_FLAG | linux.FUTEX.WAIT, | ||
0, | ||
null, | ||
))) { | ||
|
@@ -128,7 +128,7 @@ pub const AtomicCondition = struct { | |
.linux => { | ||
switch (linux.getErrno(linux.futex_wake( | ||
&cond.futex, | ||
linux.FUTEX_PRIVATE_FLAG | linux.FUTEX_WAKE, | ||
linux.FUTEX.PRIVATE_FLAG | linux.FUTEX.WAKE, | ||
1, | ||
))) { | ||
.SUCCESS => {}, | ||
|
@@ -152,9 +152,9 @@ pub const AtomicCondition = struct { | |
@atomicStore(bool, &cond.pending, true, .SeqCst); | ||
} | ||
|
||
mutex.unlock(); | ||
mutex.releaseDirect(); | ||
waiter.data.wait(); | ||
mutex.lock(); | ||
_ = mutex.acquire(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same argument as for line 155. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning here was this: since we expect a locked mutex to be passed into the
Since the mutex is guaranteed to be locked when Now if I make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes. looks good. The overall code in |
||
} | ||
|
||
pub fn signal(cond: *AtomicCondition) void { | ||
|
@@ -193,3 +193,62 @@ pub const AtomicCondition = struct { | |
waiter.data.notify(); | ||
} | ||
}; | ||
|
||
// verify that the condition variable unblocks when signalled | ||
test "AtomicCondition" { | ||
if (!builtin.single_threaded) { | ||
var wait_thread_alive = std.atomic.Atomic(bool).init(false); | ||
var wait_thread_finished = std.atomic.Atomic(bool).init(false); | ||
var run_condition = std.atomic.Atomic(bool).init(false); | ||
var condvar = Condition{}; | ||
const test_thread = try std.Thread.spawn(.{}, conditionWaitThread, .{ &wait_thread_alive, &wait_thread_finished, &run_condition, &condvar }); | ||
test_thread.detach(); | ||
|
||
// we give the waiting thread generous time to become alive, but | ||
// in case it does not, we fail here rather than hang indefinitely | ||
try waitUntilTrue(&wait_thread_alive, 10); | ||
|
||
// this does not really tell us much, but we might as well check it | ||
try std.testing.expect(!wait_thread_finished.load(.SeqCst)); | ||
|
||
run_condition.store(true, .SeqCst); | ||
condvar.signal(); | ||
|
||
// similar as above we let the thread indicate it is finished or fail | ||
try waitUntilTrue(&wait_thread_finished, 10); | ||
} | ||
} | ||
|
||
/// a primitive helper method that blocks until an atomic boolean becomes true | ||
/// if the bool does not become true within the given amount of max seconds, this function failes | ||
fn waitUntilTrue(boolean: *std.atomic.Atomic(bool), max_wait_time_seconds: u32) !void { | ||
var current_time: std.os.timespec = undefined; | ||
try std.os.clock_gettime(std.os.CLOCK.REALTIME, ¤t_time); | ||
const start_time = current_time; | ||
while (!boolean.load(.SeqCst)) { | ||
std.os.nanosleep(1, 250 * 1000); | ||
try std.os.clock_gettime(std.os.CLOCK.REALTIME, ¤t_time); | ||
if (current_time.tv_sec - start_time.tv_sec > max_wait_time_seconds) { | ||
return error.Timeout; | ||
} | ||
} | ||
} | ||
|
||
/// a helper thread for testing the condition variable. Both wait_... variables must be false when passed to the thread. | ||
/// when the thread starts it | ||
fn conditionWaitThread(wait_thread_alive: *std.atomic.Atomic(bool), wait_thread_finished: *std.atomic.Atomic(bool), run_condition: *std.atomic.Atomic(bool), condvar: *Condition) void { | ||
std.debug.assert(!wait_thread_alive.load(.SeqCst)); | ||
std.debug.assert(!wait_thread_finished.load(.SeqCst)); | ||
|
||
// indicate this thread has started up | ||
wait_thread_alive.store(true, .SeqCst); | ||
var mutex = std.Thread.Mutex{}; | ||
const held = mutex.acquire(); | ||
defer held.release(); | ||
// wait for the condition variable | ||
while (!run_condition.load(.SeqCst)) { | ||
condvar.wait(&mutex); | ||
} | ||
// and indicate that this thread has completed | ||
wait_thread_finished.store(true, .SeqCst); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock
andunlock
are implemented inRwLock.zig
. Did you check, if the code got moved from Mutex.zig into RwLock.zig and thus broke stuff? Otherwise you dont explain in the commit message why changing this is fineAlso it says
/// Release the mutex. Prefer Held.release() if available.
, so you might want to explain whyrelease
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for taking the time to review the PR. My reasoning was simple (maybe simplistic): since no instance of
Held
is available to me, I just used theMutex
API. Would it make sense to extend the interface ofwait
to include theHeld
instance? This would feel repetitive and error prone, but there's probably something I am not seeing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Held
is defined inMutex.zig
, but not needed as we dont expose it to the user. I asked aboutunlock
, but I checked the git log and nothing shows up so it looks like it was broken from the commit it was introduced.