-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Added unlock functionality to mutex guard and rw lock guards #149189
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
base: main
Are you sure you want to change the base?
Added unlock functionality to mutex guard and rw lock guards #149189
Conversation
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| unsafe { self.lock.inner.unlock() }; | ||
|
|
||
| func(); | ||
|
|
||
| self.lock.inner.lock(); |
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.
Not a fundamental design issue, but the implementation looks unsound in the presence of panics since it leaves the Mutex unlocked when exiting the function
let mutex = Mutex::new(1);
let mut guard = mutex.lock().unwrap();
panic::catch_unwind(AssertUnwindSafe(|| MutexGuard::unlocked(&mut guard, || panic!())));
let ref1 = &mut *guard;
let ref2 = &mut *mutex.lock();
dbg!(ref1, ref2);lock_api doesn't have this issue since it calls lock() in a local's Drop impl that always executes, should probably do the same here.
edit: Ah, I overlooked the new unlocked: bool field, the comment for it also talks about panicking in the closure, but from a cursory look I'm not sure it prevents this issue?
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.
Oh yes, that's unsound. I don't think the "no relock during panic" guarantee can be soundly provided, at least if unlocked only takes the mutex guard by reference.
|
|
||
| impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> { | ||
| /// Unlocks the [`MutexGuard`] for the scope of `func` and acquires it again after. | ||
| /// Panics won't lock the guard again. |
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.
How important is this guarantee? I find it unfortunate that this necessitates adding a field and an additional check in the Drop implementation, which affects all code using MutexGuards, not just code that calls unlocked.
|
r? joboet |
Added unlock functionality for nonpoison mutex guards and RwLock guards.
Associated Tracking Issue: #148568