Skip to content
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

Tracking Issue for ReentrantLock #121440

Open
2 of 4 tasks
joboet opened this issue Feb 22, 2024 · 10 comments
Open
2 of 4 tasks

Tracking Issue for ReentrantLock #121440

joboet opened this issue Feb 22, 2024 · 10 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joboet
Copy link
Contributor

joboet commented Feb 22, 2024

Feature gate: #![feature(reentrant_lock)]

This is a tracking issue for ReentrantLock, a re-entrant lock useful for avoiding a common type of deadlocks.

Public API

// std::sync

pub struct ReentrantLock<T: ?Sized> {}
pub struct ReentrantLockGuard<'a, T: ?Sized + 'a> {}

impl<T> ReentrantLock<T> {
    pub const fn new(t: T) -> ReentrantLock<T>;
    pub fn into_inner(self) -> T;
}

impl<T: ?Sized> ReentrantLock<T> {
    pub fn lock(&self) -> ReentrantLockGuard<'_, T>;
    pub fn get_mut(&mut self) -> &mut T;
}

impl<T: Send + ?Sized> Send for ReentrantLock<T> {}
impl<T: Send + ?Sized> Sync for ReentrantLock<T> {}

impl<T: ?Sized> !Send for ReentrantLockGuard<'_, T> {}
impl<T: ?Sized + Sync> Sync for ReentrantLockGuard<'_, T> {}

impl<T: UnwindSafe + ?Sized> UnwindSafe for ReentrantLock<T> {}
impl<T: RefUnwindSafe + ?Sized> RefUnwindSafe for ReentrantLock<T> {}

impl<T: Default> Default for ReentrantLock<T> {}
impl<T> From<T> for ReentrantLock<T> {}

impl<T: ?Sized> Deref for ReentrantLockGuard<'_, T> {
    type Target = T;
}

impl<T: Debug + ?Sized> Debug for ReentrantLock<T> {}
impl<T: Debug + ?Sized> Debug for ReentrantLockGuard<'_, T> {}
impl<T: Display + ?Sized> Display for ReentrantLockGuard<'_, T> {}

Steps / History

Unresolved Questions

Poisoning

I would argue that ReentrantLock should not implement lock poisoning. Since it only provides shared access, breaking the invariants of the inner type is something that the lock has no control over, as it requires interior mutability. It would make sense to require that T: RefUnwindSafe, but I think that is too inconvenient for now. If RefUnwindSafe were a lint trait, it should definitely be used.

Fallible locking

Is there a need for fallible locking, even if it cannot fail because of reentrancy? How would this look like, considering that TryLockResult also has a poisoning case, which would never be used here?

Naming

New synchronization primitives like OnceLock have used the Lock suffix. IMHO the name ReentrantLock rhymes well with the other types and is more consistent, but ReentrantMutex better highlights the similarity to Mutex.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@joboet joboet added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 22, 2024
@RalfJung
Copy link
Member

New synchronization primitives like OnceLock have used the Lock suffix. IMHO the name ReentrantLock rhymes well with the other types and is more consistent, but ReentrantMutex better highlights the similarity to Mutex.

I'll put in my vote for ReentrantMutex.

OnceLock isn't even a lock or a mutex, it's a thread-safe version of OnceCell. I don't know why it has "lock" in its name but IMO that's not a good justification for the asymmetry between Mutex and ReentrantMutex.

@joboet
Copy link
Contributor Author

joboet commented Mar 12, 2024

I'm still dreaming of a future in which we have a poison-free Lock instead of Mutex. In that future, ReentrantLock is the perfect name. But I'm not convinced that it's achievable. Still, I'd like to defer the final decision until stabilization.

@RalfJung
Copy link
Member

RalfJung commented Apr 4, 2024

I don't see why "Mutex" would implicitly mean "poisoned". That's not an obvious naming scheme at all.

@botahamec
Copy link
Contributor

If we have unpoisoned mutexes in the future, it'd probably be better to do something like CleanReentrantMutex. And maybe it could be aliased in a future edition, but that's obviously outside this issue's scope.

@Eclipse32767
Copy link

Alright question, why not have interior mutability on the ReentrantLock? I don't think there's a data race problem here, but I might be missing something

@RalfJung
Copy link
Member

You mean returning an &mut when acquiring the lock?

Then I could acquire the lock twice in the same thread, have two aliasing mutable references, and proceed to break everything from there.

@Eclipse32767
Copy link

Good catch, I didn’t notice why everything blows up with this, just noticed how interior mutability was out of the question and wondered why

@RadiantUwU
Copy link

Please make try_lock be public to be the same as Mutex and RwLock.
ReentrantLock would have more use cases if it can directly replace Mutex when an object is locked more than once.

@workingjubilee
Copy link
Member

OnceLock isn't even a lock or a mutex, it's a thread-safe version of OnceCell. I don't know why it has "lock" in its name but IMO that's not a good justification for the asymmetry between Mutex and ReentrantMutex.

It is the sort of lock you throw away the key to.

Perhaps OnceDungeon sounded too grim.

@workingjubilee
Copy link
Member

#125527 corrects the Sync impl to be something actually suitable for a publicly usable type.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 25, 2024
Rollup merge of rust-lang#125527 - programmerjake:patch-2, r=workingjubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants