How can I dereference an Arc<Mutex<Vec>>> for return?

[natecox]

I'm struggling to implement concurrency in my small learning app and I think I've hit a wall; I could really use some advice to help me understand why I'm stuck and maybe a kind soul to get me to working code.

I'm working with code in this PR and I have worked my way through many, many compiler errors which were all helpful learning experiences. The wall that I have hit is at the very end when I try to return Ok with a vector of a custom struct.

This is the offending line and it is giving me the following error:

✦ ❯ cargo run -- -c corrator.toml -f text
   Compiling corrator v0.1.1 (/Users/ncox/src/natecox/corrator)
error[E0507]: cannot move out of dereference of `MutexGuard<'_, std::vec::Vec<container::Status>>`
  --> src/lib.rs:80:8
   |
80 |     Ok(*data.lock().unwrap())
   |        ^^^^^^^^^^^^^^^^^^^^^ move occurs because value has type `std::vec::Vec<container::Status>`, which does not implement the `Copy` trait

error[E0597]: `data` does not live long enough
  --> src/lib.rs:80:9
   |
80 |     Ok(*data.lock().unwrap())
   |         ^^^^^^^^^^^---------
   |         |
   |         borrowed value does not live long enough
   |         a temporary with access to the borrow is created here ...
81 | }
   | -
   | |
   | `data` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `MutexGuard`
   |
   = note: the temporary is part of an expression at the end of a block;
           consider forcing this temporary to be dropped sooner, before the block's local variables are dropped
help: for example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block
   |
80 |     let x = Ok(*data.lock().unwrap()); x
   |     +++++++                          +++

Some errors have detailed explanations: E0507, E0597.
For more information about an error, try `rustc --explain E0507`.
error: could not compile `corrator` due to 2 previous errors

I'm not really understanding why a move is happening there so I'm not sure where to go next. I would really appreciate some help here.

[steffahn]

There are multiple approaches. From worse to better in this particular situation, as far as I can tell, but all of them useful to know in general.

Since the .lock() gives you mutable access, you can use mem::take to obtain ownership of the Vec and leave an empty vector in its place. (The even more general API for types without Default value is mem::replace.)

Since the Arc seems to be the only copy remaining, you can use Arc::try_unwrap to obtain ownership, and then unwrap the Result and use the into_inner method for the Mutex.

Since the threads are spawned, and then joined, you can avoid the need for any Arc in the first place and just use an owned Mutex value in your local data variable, accessed by shared reference from the threads which get spawned in a thread::scope context. This API even takes care of the need to collect and manually join on the handles. With an owned Mutex, all you'll need in the end is to call is the Mutex::into_inner method.

[natecox]

Ok, so I dug into the second suggestion and took ownership with Arc::try_unwrap and I have working code now, thank you for that! The execution time for my app just got cut in half and that's a pretty big win.

I think I understand why taking ownership works, but I may be too dumb to grasp how to implement the third solution of just using a Mutex without Arc. I'm not sure what you mean by " accessed by shared reference from the threads which get spawned in a thread::scope context".

[Cerber-Ursi]

@natecox

I'm not sure what you mean by " accessed by shared reference from the threads which get spawned in a thread::scope context".

That means, if you use thread::scope instead of thread::spawn, you'll be able to use &Mutex instead of Arc<Mutex>. Therefore, after the scope ends and the shared borrow is released, you have an owned Mutex already available.

[natecox]

oh haha I read right past that and didn't realize it was "scope", my bad. Ok, thanks, I'll do some digging on that too.

[steffahn]

Here’s some example refactor of the code. I’ve also adjusted a few small things to look “nicer”, and notably I’ve removed the superfluous Mutex around the applications which are only accessed immutably anyways.

pub fn run(config: Config) -> Result<Vec<container::Status>, Box<dyn Error>> {
    let data = Mutex::new(vec![]);

    thread::scope(|s| {
        for (name, container) in &config.containers {
            s.spawn(|| {
                let mut apps = container.apps.clone();
                let mut container_status = container::Status::new(name.clone());

                apps.sort();
                docker::run(name, &container.path).expect("Unable to start docker container");

                for app_name in &apps {
                    let app = &config.applications[app_name];
                    let output = docker::execute(name, &app.version_command);
                    let version = app.query_version(&output).unwrap();

                    let eol_status: String = match &app.eol {
                        Some(x) => {
                            let status: String = x.query(&version).unwrap().into();
                            format!("(eol: {status})")
                        }
                        _ => String::new(),
                    };

                    container_status.apps.push(application::Status {
                        name: app_name.clone(),
                        version,
                        eol_status: Some(eol_status),
                    });
                }

                docker::stop(name).expect("Unable to clean up docker container");

                let mut data = data.lock().unwrap();
                data.push(container_status);
            });
        }
    });

    Ok(data.into_inner().unwrap())
}

Making the code work with a non-move closures involved some deeper understanding of closure capturing (e.g. using “name” instead of “&name” in places like the docker::run argument, so that name is not borrowed, but only *name is borrowed); if you want to use the thread::scope approach with a move closure instead (e.g. if the code changes and you want ownership of the container, so you want to iterate by-value again over config.containers), you might need to define a variable containing the references you are interested in, e.g. let data_ref = &data;, so that data_ref can be moved into the spawned closure (and maybe the same thing for a reference to container.applications).

Edit: On second thought, I think I want ownership so I can avoid the clone for container.apps. I’ll post an alternative version shortly.

Edit2: I didn’t need a move closure still because I could make sure that the (name, container) pair is moved nonetheless:

pub fn run(config: Config) -> Result<Vec<container::Status>, Box<dyn Error>> {
    let data = Mutex::new(vec![]);

    thread::scope(|s| {
        for entry in config.containers {
            s.spawn(|| {
                let (name, mut container) = entry;
                let mut container_status = container::Status::new(name.clone());

                container.apps.sort();
                docker::run(&name, &container.path).expect("Unable to start docker container");

                for app_name in container.apps {
                    let app = &config.applications[&app_name];
                    let output = docker::execute(&name, &app.version_command);
                    let version = app.query_version(&output).unwrap();

                    let eol_status: String = match &app.eol {
                        Some(x) => {
                            let status: String = x.query(&version).unwrap().into();
                            format!("(eol: {status})")
                        }
                        _ => String::new(),
                    };

                    container_status.apps.push(application::Status {
                        name: app_name,
                        version,
                        eol_status: Some(eol_status),
                    });
                }

                docker::stop(&name).expect("Unable to clean up docker container");

                let mut data = data.lock().unwrap();
                data.push(container_status);
            });
        }
    });

    Ok(data.into_inner().unwrap())
}

This now avoids cloning of container.apps as well as app_name.

[natecox]

Thanks for this, I was so very close to basically exactly this but I got caught up on syntax on the locking line and thought I probably needed to sleep on it, was banging my head against this for hours. I feel better knowing that I wasn't horrifically far off.