-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
make stderr mutex recursive and lock it when panicking #20268
Conversation
044d604
to
b62b0f3
Compare
I realized that what I'm describing is simply making the stderr mutex a recursive mutex. |
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.
b62b0f3
to
2cca9de
Compare
2cca9de
to
fad223d
Compare
lib/std/Thread/Mutex/Recursive.zig
Outdated
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); |
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.
Think this could be simplified:
lock():
c = LOAD(&count, Unordered)
if not (c > 0 and LOAD(¤t_tid, Unorderd) == tid):
mutex.lock() // make this failable for tryLock()
c = LOAD(&count, Unordered)
assert(c == 0)
STORE(¤t_id, tid, Unordered)
STORE(&count, c + 1, Unordered)
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.
TBH, an invalid_thread_id would be simplest:
lock():
if LOAD(¤t_tid, Unordered) != tid:
mutex.lock()
STORE(¤t_tid, tid, Unordered)
count += 1
unlock():
count -= 1
if count == 0:
STORE(¤t_tid, invalid_tid, Unordered)
mutex.unlock()
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.
Nice that last impl looks pretty good.
This version is simpler. Thanks King!
This patch adds
std.Thread.Mutex.Recursive
which is a recursive mutex implementation (cc @kprotty for review).Next, it changes the mutex used by
std.debug.lockStdErr()
to be a recursive mutex, and obtains the lock when panicking.The previously used
panic_mutex
is no longer needed.