Is this a bug?(unlock is too late...)

use std::thread;
use std::sync::{Arc, Mutex};
use std::sync::mpsc;

fn main() {
    let data = Arc::new(Mutex::new(0));
    for _ in 0..10 {
        let local_data = data.clone();
        thread::spawn(move || {
            println!("start............");
            let mut th_data = local_data.lock().unwrap();
            *th_data += 1;
            println!("Unlocking the data here is best.");
            thread::sleep_ms(5000); //do some job cost a lot of time.
            println!("Unlocking here is too late.");
        });
    }
    thread::sleep_ms(50000);
}

the shared data is too late to unlock....
except only cost a little more than 5s, but actually, cost more than 50s...

No, that is not a bug. Drops happen at the end of the scope or when you give up ownership.

You can avoid this by explicitly dropping the lock:

let mut th_data = local_data.lock().unwrap();
*th_data += 1;
drop(th_data);

Or alternatively, move the operation into a (possibly inline-able) function:

fn do_my_stuff(data: &Mutex<i32>) {
    let mut th_data = local_data.lock().unwrap();
    *th_data += 1;
}

A MutexGuard (return value of lock) only unlocks the Mutex in its destructor, which only runs when the value goes out of scope. In this case, that is the }); line.

You can explicitly move a value out of scope and hence runs its destructor using the drop function:

thread::spawn(move || {
    println!("start............");

    let mut th_data = local_data.lock().unwrap();
    *th_data += 1;

    drop(th_data); // this line

    println!("Unlocking the data here is best.");
    thread::sleep_ms(5000); //do some job cost a lot of time.
    println!("Unlocking here is too late.");
});

Alternatively, you can introduce a smaller scope, so that the th_data variable is only live for a shorter period:

thread::spawn(move || {
    println!("start............");

    {
        let mut th_data = local_data.lock().unwrap();
        *th_data += 1;
    } // th_data goes out of scope here

    println!("Unlocking the data here is best.");
    thread::sleep_ms(5000); //do some job cost a lot of time.
    println!("Unlocking here is too late.");
});
2 Likes

I'm new to rust..thanks.
I have thought rust's dropping happens at the end of use.

Which do you think is the preferred style? Explicitly dropping the guard or using an inner scope? I've used both and I couldn't say which I prefer, though the scope solution makes it clear when the guard is active.

I have a weirdly irrational distaste for mem::drop.

Well, it's not totally irrational. It doesn't relinquish borrows like a scope does.

1 Like

Yeah, that's one thing that still bugs me about dropck semantics. You'd think that moving a borrowed value into a function would make its lifetime end after that function returns. Is it just too much work to determine that?

The difference is that mem::drop gives you a (limited) ability to decide lifetime at runtime.

use std::mem;
struct Noisy;
impl Drop for Noisy {
    fn drop(&mut self) {
        println!("dropped");
    }
}

fn demo(flag: bool) {
    let z = Noisy;
    if flag {
        mem::drop(z);
    }
    println!("may be before or after drop");
}

fn main() {
    demo(false);
    demo(true);
}

If you don't need that, use an inner scope – it's fully static and makes the borrow checker happier.

1 Like

Non-lexical lifetimes AKA SEME regions would solve this. Reportedly "falls out" of MIR, so hopefully not long!

In my opinion drop or even introducing inner scopes is a good sign that you are embedding a concern in a function that should be somewhere else. (a separate function)

1 Like