Tracking Issue for Mutex::unlock()

[mark-i-m]

Feature gate: #![feature(mutex_unlock)]

This is a tracking issue for the Mutex::unlock function. The goal of this function is to replace drop(mutex_guard) which doesn't clear express that a synchronization event is occurring.

Some earlier discussion, including a previous implementation: #79434 (comment)

Public API

// std::sync::Mutex

impl<T> Mutex<T> {
    /// Immediately drops the guard, unlocking the mutex.
    ///
    /// This is equivalent to `drop(guard)`, but it is more self-documenting.
    pub fn unlock(guard: MutexGuard<T>) {
        drop(guard);
    }
}

Steps / History

• Implementation: Add Mutex::unlock #81873
• Final commenting period (FCP)
• Stabilization PR

Unresolved Questions

• Should an analogous function be added to RwLock? It would be a bit more complicated.

[mbrubeck]

I don't think this needs to require T: Sized. Should it be moved to impl<T: ?Sized> Mutex<T>?

[mark-i-m]

I can make a PR for that...

[ibraheemdev]

A bit pedantic, but wouldn't it make sense for the drop impl for MutexGuard to call unlock, and the logic to be moved there?

[mbrubeck]

A bit pedantic, but wouldn't it make sense for the drop impl for MutexGuard to call unlock, and the logic to be moved there?

MutexGuard::drop takes &mut self, so it can't easily call unlock, which consumes the MutexGuard by value.

[ibraheemdev]

Ah right, that is problematic.

[frogtd]

The drop function call is useless BTW, but it could make it slightly more clear.

[StrongerXi]

Any progress on this? Wondering why it's still pending...

[mark-i-m]

Mainly, I think this is lacking manpower to drive it forward...

[jw2476]

What needs to be done to get this into stable?

[BurntSushi]

My own inclination is to not accept this. It seems likely to cause folks to use this when it would just be better to let drop handle it. It's idiomatic for drop to deal with these sorts of things IMO, and I'm not convinced that the self-documenting nature of this method is enough to motivate setting a precedent for this.

Curious to hear what the rest of the team thinks.

@rfcbot fcp close

[rfcbot]

Team member @BurntSushi has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[mark-i-m]

@BurntSushi Reading back on the thread, I realize the motivation is not really discussed.

I have a very particular use cases in mind. I agree that normally, it should be idiomatic to just let the lock drop by going out of scope. However, there are cases when this is not preferable, and currently one must call drop(guard) to get the desired behavior, which is a non-obvious way to indicate a lock being released.

1. Avoiding deadlocks. Example:

   let locked1 = my_btree.lock().unwrap();
   // do a bunch of stuff with locked1
   
   let locked2 = my_other_btree.lock().unwrap();
   // do a bunch of stuff with locked2 
   
   wait_for_other_thread();
   // OOPS! Deadlock! Other thread needs to acquire the locks.

2. Performance

   let locked1 = my_btree.lock().unwrap();
   // do stuff with my_btree
   
   compute_something_expensive();
   
   // OOPS! All other threads blocked while we compute something expensive!

3. Documentation -- in long functions, the only way I know of to create a short critical section is the use a new { } block or use drop(guard), neither of which seems great... they're hard to see/search for; the { } approach causes rightward drift, etc.

   fn really_long_fn() {
      // do a bunch of stuff      
   
      {
        let locked = my_data.lock().unwrap();
        // do stuff
      } // unlock
   
      // more stuff
   
     {
        // Another critical section
      } 
   
      // more stuff
   
      // Another critical section (alternately using braces)
      let locked = my_data.lock().unwrap();
      // do stuff
      drop(locked);
   }

Anyway, I hope that's useful.

[BurntSushi]

Yeah that all makes sense. I'm just not sure it's worth adding an explicit unlock for those use cases instead of focusing effort on drop(locked). It's less direct, but drop is a critical part of how Rust works and it's important for folks to grapple with it. There's similar ideas in place with things like File and other guards, like those for RefCell.

[mark-i-m]

It's less direct, but drop is a critical part of how Rust works and it's important for folks to grapple with it. There's similar ideas in place with things like File and other guards, like those for RefCell.

@BurntSushi Do those cases have performance or correctness implications? I guess RefCell could have an analogous problem to deadlocks. I guess I'm arguing that locks are significantly more common to have to manually drop than the others and that the impact is significantly more difficult to debug, as to make it deserve some special treatment. (That might be entirely because of the types of projects I work on, though).

instead of focusing effort on drop(locked)

I'm intrigued. What kind of efforts are you thinking of? How do you see manual drop being improved? I'm open to other solutions.

[dtolnay]

FWIW I like this method. I implemented Mutex::unlock, with the same signature, for crosvm's Mutex in Chrome OS more than a year before the same method was proposed for the standard library: https://chromium.googlesource.com/chromiumos/platform/crosvm/+/b70614d2526a562ac7a6eae4b734198908aafdec%5E%21. But I also sympathize with the point that it will inevitably get used in situations it definitely shouldn't be. Normally I am the one making that argument, such as for #80437.

[tgross35]

Relevant zulip thread https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/stabilizing.20Mutex.3A.3Aunlock.28.29

It was brought up there that maybe some closure-based API (some_mutex.with_lock(|g| ... )) would be a good way to express guard scope when the default drop time isn't ideal or isn't clear. I believe that is the case this PR seeks to address.

I have to agree with that sentiment, especially since many times you only need to run one or two statements while a mutex is locked. Encouraging use of a closure to perform small-scoped operations seems like very idiomatic Rust. Having a wrapper around an existing public API that doesn't add new functionality - less so.

[mark-i-m]

@tgross35 I do think that a closure-based interface like that can be useful, but it also has some big limitations:

• it increases rightward drift, especially if you have multiple locks.
• creating the closure often requires moving or borrowing other variables in the environment, which might be painful or inefficient (e.g. if they are not Copy or Clone types).
• closures mess with ?, break, continue, and other control-flow constructs.

[tgross35]

Yeah, there are of course problems and I see it was discussed in #81873. My point was moreso just that I don't think it would make sense to have both this and a (RFC reviewed) closure method, since they kind of target similar use cases. And for the remaining 15% of cases, drop or { scope } would work fine. But I'm a spectator and that's just mho.

For what it's worth on rightward drift with >1 mutex, I think most cases could get by sharing the scope of the first mutex.

mtx1.with_lock(|guard1| {
    let guard2 = mtx2.lock().unwrap();
    let guard3 = mtx3.lock().unwrap();
}); // lock it all back up

[Amanieu]

See #61976 for a previous attempt to add Mutex::with.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.