Book: MutexGuard unlocking not explained accurately enough.

[xStrom]

This issue has been brought up before in #972 by @baumanj.

Now chapter 20.2, at the end, talks about why using loop is better than while so that the MutexGuard gets unlocked before the expensive job.call_box() call. Specifically: _By using loop instead and acquiring the lock and a job within the block rather than outside it, the MutexGuard returned from the lock method is dropped as soon as the let job statement ends._

Acquiring the lock inside the loop block does not guarantee that the MutexGuard is dropped after the let job statement! It's dropped because the MutexGuard doesn't get assigned a variable name, i.e. it's a temporary variable.

loop {
    // We chain call recv() immediately on the temporary MutexGuard
    let job = receiver.lock().unwrap().recv().unwrap(); 
    // MutexGuard is dropped, because it didn't get assigned to any variable

    println!("Worker {} got a job; executing.", id);

    job.call_box();
}

loop {
    // We assign MutexGuard to a variable
    let mg = receiver.lock().unwrap();
    let job = mg.recv().unwrap();
    // Although the MutexGuard is never referenced again,
    // it won't be dropped until the end of the scope

    println!("Worker {} got a job; executing.", id);

    job.call_box();
} // MutexGuard finally dropped

I think it's important to add a note to the book that the MutexGuard is dropped due to not being assigned to a variable. I can also see value in rewriting it with an explict drop call.

Because frankly all of this seems pretty unsafe. How stable & strict are these compiler contracts of when things get dropped? Can the drop calls be lazily executed sometimes? There's some info in the reference and there's a nice playground example.

Always explicitly calling drop on any synchronization objects seems to be a smart move, because otherwise subtle issues & inefficiencies can start creeping in where locks are held longer than expected. When someone changes code by introducing a named variable of a mutex, I doubt it's going to be obvious to most that it'll introduce extended lock holding. This seems like something only a minority of Rustaceans will know, much less catch in a code review. Add on that the risks of the compiler optimizations being changed on exact drop call timings. Being explicit seems worth it.

[mingxinwei]

Cant agree more. I was quite confused at there.

the MutexGuard returned from the lock method is dropped as soon as the let job statement ends.

After seeing this, all I think is that MutexGuard is dropped at the end of the loop, where it go out of scope. This makes me doubt on what Rust advertises it can do. A slightly (but quite common) change makes the multi thread example fall back to single thread. That's not RUST at all.

[UnHumbleBen]

Thanks for bringing this issue up, but I have trouble understanding your explanation that MutexGuard is dropped due to not being assigned to any variables. In your example, it makes sense since MutexGuard is explicitly assigned to mg.

let mg = receiver.lock().unwrap();

So how come using while let doesn't work then? As shown in the book,

while let Ok(job) = receiver.lock().unwrap().recv() {

There is no variable assigned to MutexGuard. Shouldn't MutexGuard also be dropped?

[xStrom]

@UnHumbleBen great question! I did some testing to get more info and you can check out the playground code.

It seems that in the while loop case temporary variables aren't dropped either until the end of the loop scope, if there's a named variable containing a result from one of its methods.

// The result of getFoo() never gets assigned a variable,
// but we store the result of its num() method in a named variable n.
while let n = getFoo().num() {
    break;
} // The result of getFoo() gets dropped here

// The result of getFoo() never gets assigned a variable,
// and neither does the result of num() which we compare anonymously.
while getFoo().num() != 1 {
    // The result of getFoo() is already dropped here
    break;
}

I'm more and more convinced that locks (or anything else where drop timing matters) should always have explicit drop calls, because the implicit drop mechanics seem to be pretty varied and aren't properly documented anywhere.

[baumanj]

Hey, @steveklabnik this is the issue I mentioned to you at RustConf (at the end of the morning training).

I posted https://users.rust-lang.org/t/drop-semantics-and-temporary-value-lifetimes-in-while-let/30903, which led to some interesting discussion and ultimately the conclusion that let and if let bindings have surprisingly different behavior. As someone who's been involved with the community for a long time, I'm interested in your take on this. Do you think it could ever be changed (perhaps in an edition), or are we stuck with unfortunately inconsistent behavior we have now?