Skip to content

Conversation

@SpaceBroetchen
Copy link

@SpaceBroetchen SpaceBroetchen commented Nov 21, 2025

Added unlock functionality for nonpoison mutex guards and RwLock guards.

Associated Tracking Issue: #148568

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2025

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@SpaceBroetchen SpaceBroetchen marked this pull request as draft November 21, 2025 18:47
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Comment on lines +537 to +541
unsafe { self.lock.inner.unlock() };

func();

self.lock.inner.lock();
Copy link
Member

@y21 y21 Nov 25, 2025

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?

Copy link
Member

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.
Copy link
Member

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.

@ChrisDenton
Copy link
Member

r? joboet

@rustbot rustbot assigned joboet and unassigned ChrisDenton Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants