Re: [PATCH v4] rust: lock: Export Guard::do_unlocked()

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Oct 30, 2025 at 6:41 PM Lyude Paul <lyude@redhat.com> wrote:
>
> On Thu, 2025-10-30 at 11:43 +0100, Paolo Bonzini wrote:
> > On 10/29/25 19:35, Lyude Paul wrote:
> > > +    /// // Since we hold work.lock, which work will also try to acquire in WorkItem::run. Dropping
> > > +    /// // the lock temporarily while we wait for completion works around this.
> > > +    /// g.do_unlocked(|| work.done.wait_for_completion());
> > > +    ///
> > > +    /// assert_eq!(*g, 42);
> > > +    /// ```
> > > +    pub fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
> > >           // SAFETY: The caller owns the lock, so it is safe to unlock it.
> > >           unsafe { B::unlock(self.lock.state.get(), &self.state) };
> >
> > Getting self as &mut is incorrect.  That's because owning a lock guard
> > implicitly tells you that no other thread can observe the intermediate
> > states of the object.  (The same is even more obviously true for a
> > RefCell's mutable borrow, i.e. core::cell::RefMut)
> >
> > Let's say you have a lock-protected data structure with an invariant
> > that is preserved at the end of every critical section.  Let's say also
> > that you have a function
> >
> >      fn do_something() {
> >          let g = self.inner.lock();
> >          g.mess_up_the_invariant();          // (1)
> >          self.do_something_else(&mut g);     // uses do_unlocked()
> >          g.fix_the_invariant();              // (2)
> >      }
> >
> > Because the function holds a guard between the calls (1) and (2), it
> > expects that other thread cannot observe the temporary state.  The fact
> > that do_unlocked() takes a &mut doesn't help, because the common case
> > for RAII objects is that they're passed around mutably.
> >
> > Instead, do_unlocked should take the guard and return another one:
> >
> >      fn do_something() {
> >          let mut g = self.inner.lock();
> >          g.mess_up_the_invariant();          // (1)
> >          g = self.do_something_else(g);      // uses do_unlocked()
> >          g.fix_the_invariant();              // (2)
> >      }
> >
> > This version of the interface makes it clear that (1) and (2) are in a
> > separate critical section.  Unfortunately it makes the signature uglier
> > for do_unlocked() itself:
> >
> >      #[must_use]
> >      pub fn do_unlocked<U>(self, cb: impl FnOnce() -> U) -> (Self, U)
>
> Hm, it seems then that we should probably fix this before exporting it then!
> Thank you for pointing this out, I'll fix it in the next respin.

I do agree that this behavior has a lot of potential to surprise
users, but I don't think it's incorrect per se. It was done
intentionally for Condvar, and it's not unsound. Just surprising.

Of course, that doesn't mean we can't change it. Condvar could be
updated to use ownership in that way, and doing say may be a good
idea.

Alice