From 55a9ea250cf2aad58f3c4eb49ac6ee8d2b6f5cff Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 11 Jun 2024 15:14:06 -0700 Subject: [PATCH 1/4] std.debug: lock stderr mutex when panicking The doc comments for this global said: "Locked to avoid interleaving panic messages from multiple threads." Huh? There's already a mutex for that, it's the stderr mutex. Lock that one instead. --- lib/std/debug.zig | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/std/debug.zig b/lib/std/debug.zig index f431d0eb91..e7d0d23766 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -447,9 +447,6 @@ pub fn panicExtra( /// The counter is incremented/decremented atomically. var panicking = std.atomic.Value(u8).init(0); -// Locked to avoid interleaving panic messages from multiple threads. -var panic_mutex = std.Thread.Mutex{}; - /// Counts how many times the panic handler is invoked by this thread. /// This is used to catch and handle panics triggered by the panic handler. threadlocal var panic_stage: usize = 0; @@ -474,8 +471,8 @@ pub fn panicImpl(trace: ?*const std.builtin.StackTrace, first_trace_addr: ?usize // Make sure to release the mutex when done { - panic_mutex.lock(); - defer panic_mutex.unlock(); + lockStdErr(); + defer unlockStdErr(); const stderr = io.getStdErr().writer(); if (builtin.single_threaded) { @@ -2604,8 +2601,8 @@ fn handleSegfaultPosix(sig: i32, info: *const posix.siginfo_t, ctx_ptr: ?*anyopa _ = panicking.fetchAdd(1, .seq_cst); { - panic_mutex.lock(); - defer panic_mutex.unlock(); + lockStdErr(); + defer unlockStdErr(); dumpSegfaultInfoPosix(sig, code, addr, ctx_ptr); } @@ -2680,8 +2677,8 @@ fn handleSegfaultWindowsExtra( _ = panicking.fetchAdd(1, .seq_cst); { - panic_mutex.lock(); - defer panic_mutex.unlock(); + lockStdErr(); + defer unlockStdErr(); dumpSegfaultInfoWindows(info, msg, label); } From 506b3f6db6c13bcced94c80ddf9bd871fb721cc0 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 12 Jun 2024 17:16:34 -0700 Subject: [PATCH 2/4] introduce std.Thread.Mutex.Recursive --- lib/std/Thread/Mutex.zig | 42 ++++++++------- lib/std/Thread/Mutex/Recursive.zig | 86 ++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 18 deletions(-) create mode 100644 lib/std/Thread/Mutex/Recursive.zig diff --git a/lib/std/Thread/Mutex.zig b/lib/std/Thread/Mutex.zig index b6d3d6fb84..11c6386569 100644 --- a/lib/std/Thread/Mutex.zig +++ b/lib/std/Thread/Mutex.zig @@ -1,23 +1,11 @@ -//! Mutex is a synchronization primitive which enforces atomic access to a shared region of code known as the "critical section". -//! It does this by blocking ensuring only one thread is in the critical section at any given point in time by blocking the others. +//! Mutex is a synchronization primitive which enforces atomic access to a +//! shared region of code known as the "critical section". +//! +//! It does this by blocking ensuring only one thread is in the critical +//! section at any given point in time by blocking the others. +//! //! Mutex can be statically initialized and is at most `@sizeOf(u64)` large. //! Use `lock()` or `tryLock()` to enter the critical section and `unlock()` to leave it. -//! -//! Example: -//! ``` -//! var m = Mutex{}; -//! -//! { -//! m.lock(); -//! defer m.unlock(); -//! // ... critical section code -//! } -//! -//! if (m.tryLock()) { -//! defer m.unlock(); -//! // ... critical section code -//! } -//! ``` const std = @import("../std.zig"); const builtin = @import("builtin"); @@ -30,6 +18,8 @@ const Futex = Thread.Futex; impl: Impl = .{}, +pub const Recursive = @import("Mutex/Recursive.zig"); + /// Tries to acquire the mutex without blocking the caller's thread. /// Returns `false` if the calling thread would have to block to acquire it. /// Otherwise, returns `true` and the caller should `unlock()` the Mutex to release it. @@ -312,3 +302,19 @@ test "many contended" { try testing.expectEqual(runner.counter.get(), num_increments * num_threads); } + +// https://github.com/ziglang/zig/issues/19295 +//test @This() { +// var m: Mutex = .{}; +// +// { +// m.lock(); +// defer m.unlock(); +// // ... critical section code +// } +// +// if (m.tryLock()) { +// defer m.unlock(); +// // ... critical section code +// } +//} diff --git a/lib/std/Thread/Mutex/Recursive.zig b/lib/std/Thread/Mutex/Recursive.zig new file mode 100644 index 0000000000..707b67f105 --- /dev/null +++ b/lib/std/Thread/Mutex/Recursive.zig @@ -0,0 +1,86 @@ +//! A synchronization primitive enforcing atomic access to a shared region of +//! code known as the "critical section". +//! +//! Equivalent to `std.Mutex` except it allows the same thread to obtain the +//! lock multiple times. +//! +//! A recursive mutex is an abstraction layer on top of a regular mutex; +//! therefore it is recommended to use instead `std.Mutex` unless there is a +//! specific reason a recursive mutex is warranted. + +const std = @import("../../std.zig"); +const Recursive = @This(); +const Mutex = std.Thread.Mutex; +const assert = std.debug.assert; + +mutex: Mutex, +thread_id: std.Thread.Id, +lock_count: usize, + +pub const init: Recursive = .{ + .mutex = .{}, + .thread_id = invalid_thread_id, + .lock_count = 0, +}; + +/// Acquires the `Mutex` without blocking the caller's thread. +/// +/// Returns `false` if the calling thread would have to block to acquire it. +/// +/// Otherwise, returns `true` and the caller should `unlock()` the Mutex to release it. +pub fn tryLock(r: *Recursive) bool { + const current_thread_id = std.Thread.getCurrentId(); + return tryLockInner(r, current_thread_id); +} + +/// Acquires the `Mutex`, blocking the current thread while the mutex is +/// already held by another thread. +/// +/// The `Mutex` can be held multiple times by the same thread. +/// +/// Once acquired, call `unlock` on the `Mutex` to release it, regardless +/// of whether the lock was already held by the same thread. +pub fn lock(r: *Recursive) void { + const current_thread_id = std.Thread.getCurrentId(); + if (!tryLockInner(r, current_thread_id)) { + r.mutex.lock(); + assert(r.lock_count == 0); + r.lock_count = 1; + @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .monotonic); + } +} + +/// Releases the `Mutex` which was previously acquired with `lock` or `tryLock`. +/// +/// It is undefined behavior to unlock from a different thread that it was +/// locked from. +pub fn unlock(r: *Recursive) void { + r.lock_count -= 1; + if (r.lock_count == 0) { + // Prevent race where: + // * Thread A obtains lock and has not yet stored the new thread id. + // * Thread B loads the thread id after tryLock() false and observes stale thread id. + @atomicStore(std.Thread.Id, &r.thread_id, invalid_thread_id, .seq_cst); + r.mutex.unlock(); + } +} + +fn tryLockInner(r: *Recursive, current_thread_id: std.Thread.Id) bool { + if (r.mutex.tryLock()) { + assert(r.lock_count == 0); + r.lock_count = 1; + @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .monotonic); + return true; + } + + const locked_thread_id = @atomicLoad(std.Thread.Id, &r.thread_id, .monotonic); + if (locked_thread_id == current_thread_id) { + r.lock_count += 1; + return true; + } + + return false; +} + +/// A value that does not alias any other thread id. +const invalid_thread_id: std.Thread.Id = 0; From fad223d92ed34caac163695cc2a32cba80267c32 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 12 Jun 2024 17:16:42 -0700 Subject: [PATCH 3/4] std.Progress: use a recursive mutex for stderr --- lib/std/Progress.zig | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/std/Progress.zig b/lib/std/Progress.zig index c7dffe4c3f..ef9099be99 100644 --- a/lib/std/Progress.zig +++ b/lib/std/Progress.zig @@ -521,6 +521,8 @@ fn windowsApiUpdateThreadRun() void { /// Allows the caller to freely write to stderr until `unlockStdErr` is called. /// /// During the lock, any `std.Progress` information is cleared from the terminal. +/// +/// The lock is recursive; the same thread may hold the lock multiple times. pub fn lockStdErr() void { stderr_mutex.lock(); clearWrittenWithEscapeCodes() catch {}; @@ -1378,4 +1380,7 @@ const have_sigwinch = switch (builtin.os.tag) { else => false, }; -var stderr_mutex: std.Thread.Mutex = .{}; +/// The primary motivation for recursive mutex here is so that a panic while +/// stderr mutex is held still dumps the stack trace and other debug +/// information. +var stderr_mutex = std.Thread.Mutex.Recursive.init; From 5fc1f8a32bb7d66c0db04e497b89f7e33f408722 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 12 Jun 2024 18:07:39 -0700 Subject: [PATCH 4/4] std.Thread.Mutex.Recursive: alternate implementation This version is simpler. Thanks King! --- lib/std/Thread/Mutex/Recursive.zig | 38 ++++++++++-------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/lib/std/Thread/Mutex/Recursive.zig b/lib/std/Thread/Mutex/Recursive.zig index 707b67f105..d6d90ed648 100644 --- a/lib/std/Thread/Mutex/Recursive.zig +++ b/lib/std/Thread/Mutex/Recursive.zig @@ -30,7 +30,13 @@ pub const init: Recursive = .{ /// Otherwise, returns `true` and the caller should `unlock()` the Mutex to release it. pub fn tryLock(r: *Recursive) bool { const current_thread_id = std.Thread.getCurrentId(); - return tryLockInner(r, current_thread_id); + if (@atomicLoad(std.Thread.Id, &r.thread_id, .unordered) != current_thread_id) { + if (!r.mutex.tryLock()) return false; + assert(r.lock_count == 0); + @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .unordered); + } + r.lock_count += 1; + return true; } /// Acquires the `Mutex`, blocking the current thread while the mutex is @@ -42,12 +48,12 @@ pub fn tryLock(r: *Recursive) bool { /// of whether the lock was already held by the same thread. pub fn lock(r: *Recursive) void { const current_thread_id = std.Thread.getCurrentId(); - if (!tryLockInner(r, current_thread_id)) { + if (@atomicLoad(std.Thread.Id, &r.thread_id, .unordered) != current_thread_id) { r.mutex.lock(); assert(r.lock_count == 0); - r.lock_count = 1; - @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .monotonic); + @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .unordered); } + r.lock_count += 1; } /// Releases the `Mutex` which was previously acquired with `lock` or `tryLock`. @@ -57,30 +63,10 @@ pub fn lock(r: *Recursive) void { pub fn unlock(r: *Recursive) void { r.lock_count -= 1; if (r.lock_count == 0) { - // Prevent race where: - // * Thread A obtains lock and has not yet stored the new thread id. - // * Thread B loads the thread id after tryLock() false and observes stale thread id. - @atomicStore(std.Thread.Id, &r.thread_id, invalid_thread_id, .seq_cst); + @atomicStore(std.Thread.Id, &r.thread_id, invalid_thread_id, .unordered); r.mutex.unlock(); } } -fn tryLockInner(r: *Recursive, current_thread_id: std.Thread.Id) bool { - if (r.mutex.tryLock()) { - assert(r.lock_count == 0); - r.lock_count = 1; - @atomicStore(std.Thread.Id, &r.thread_id, current_thread_id, .monotonic); - return true; - } - - const locked_thread_id = @atomicLoad(std.Thread.Id, &r.thread_id, .monotonic); - if (locked_thread_id == current_thread_id) { - r.lock_count += 1; - return true; - } - - return false; -} - /// A value that does not alias any other thread id. -const invalid_thread_id: std.Thread.Id = 0; +const invalid_thread_id: std.Thread.Id = std.math.maxInt(std.Thread.Id);