A note on Mutex Lock drop timing

#1

This is less of a question and more of a discovery which I thought might be useful to others. Given:

struct Foo { ... }

impl Foo {
  fn action(&mut self) { ... }
}

The following will not deadlock:

fn it_wont_deadlock() {
  let foo = Arc::new(Mutex::new(Foo { value: 0 }));

  (0..10).into_par_iter().for_each_with(foo, |f, _| {
      f.lock().unwrap().action();  // Lock immediately dropped here.
      f.lock().unwrap().action();
  }
}

But this sure will!

fn deadlock() {
  let foo = Arc::new(Mutex::new(Foo { value: 0 }));

  (0..10).into_par_iter().for_each_with(foo, |f, _| {
      let lock = f.lock().unwrap();
      f.action();
      f.lock().unwrap().action();
  }
}

The former behaviour was a surprise to me, I thought the lock would stick around until the end of the block, even though I never assigned it.

1 Like
#2

Here's the relevant reference on temporary scopes, but do note the section about temporary lifetime extension on the same page.

4 Likes
#3

I still prefer an explicit (and documented) scope for each lock:

fn no_deadlock() {
  let foo = Arc::new(Mutex::new(Foo { value: 0 }));

  (0..10).into_par_iter().for_each_with(foo, |f, _| {
      { // first mutex lock
          let lock = f.lock().unwrap();
          f.action();
      }
      { // second mutex lock
          f.lock().unwrap().action();
      }
  }
}
#4

Generally values not assigned to a variable stick around until the semicolon.

Regarding @s3bk's explicit scopes, I prefer to explicitly drop it with drop.

fn deadlock() {
  let foo = Arc::new(Mutex::new(Foo { value: 0 }));

  (0..10).into_par_iter().for_each_with(foo, |f, _| {
      let lock = f.lock().unwrap();
      f.action();
      let lock2 = f.lock().unwrap().action();
      drop(lock);
      drop(lock2);
  }
}

Now it's obvious that there is a deadlock.

4 Likes
#5

I'd like to elaborate on the temporary lifetime extension mentioned above. You should watch out for storing values borrowed from a mutex, as this can easily lead to an unexpected deadlock:

let mutex = Mutex::new(Foo { x: 42 });

// Fine: we modify the value inside the mutex,
// then the mutex unlocks immediately.
mutex.lock().unwrap().x += 1;
mutex.lock().unwrap().x += 1;

// Fine: we copy the value from the mutex,
// then the mutex unlocks immediately.
// (Value inside the mutex is not changed.)
let mut a = mutex.lock().unwrap().x;
a += 1;
let mut b = mutex.lock().unwrap().x;
b += 1;

// Deadlock: we borrow the value from the mutex guard,
// and the mutex stays locked until the end of scope of `c`.
let c = &mut mutex.lock().unwrap().x;
*c += 1;
let d = &mut mutex.lock().unwrap().x;
*d += 1;

I prefer to separate locking from accessing to avoid this pitfall. For example, this is dangerous because you have to look at the signature of action() to figure out if the mutex unlocks immediately or not:

let something = mutex.lock().unwrap().action();

This is better:

let guard = mutex.lock().unwrap();
let something = guard.action();

Now the mutex is unlocked when guard is dropped, which is much more predictable.

2 Likes
#6

Another related behaviour is _ = xxx vs _foo = xxx. I used to think that they behave the same way but the former is a temporary and the latter is an unnamed variable (don't think that's the right terminology). Consider:

let _ = f.lock().unwrap();
// ...

and

let _lock = f.lock().unwrap();
// ...

The former will unlock immediately, the latter will keep the lock until the end of scope.

Personally I prefer an explicit drop(lock) if I need to deal with explicit locks, it's clear what's happening, all variables are named and I can unlock a lock before the end of scope too. When using mutexes I generally use the guard pattern, so far I only needed explicit lock/drops when using semaphores.

3 Likes
#7

I'd call _ a non-binding, like saying "ignore this part," whereas _foo is a normal binding that also has the convention of silencing "unused" warnings.

The difference with _ is more apparent in patterns where you're moving or copying only part of something that's already bound elsewhere, instead of a temporary expression. For example, if you have some tuple x: (Vec<u32>, Vec<u32>), then let (first, _) = x; will move x.0 and leave x.1 in place, whereas let (first, _second) = x will move both parts.

1 Like
#8

Thanks, that makes sense.

Is there a good use for let _ = foo();? By the sound of it it's equivalent of calling foo() and immediately discarding the result. I guess this was the main reason for my assumption and confusion, I thought that let will always assign the value somewhere on the stack, it'd be just unnamed variable.

#9

I have #![deny(unused_must_use)], which means I must explicitly handle Results. Writing let _ = foo()?; let's me ignore a Result in those few cases where it makes sense.

#10

Isn't it defeating the purpose of the unused_must_use? You're not using the result, you're just discarding it in a workaround way. Wouldn't it suffice to foo().unwrap(); or foo()?; to explicitly handle the Result?

#11

As I said, "in the few cases where it makes sense." I want to know if I'm not consuming the Result almost all the time, but there are a few cases where I don't care.

#12

A typical case where you don't care whether an operation failed can be sending on a channel. When dealing with channels, the correct failure handling can be quite varied, and just ignoring the error is reasonably common as the correct answer.

2 Likes