[SOLVED] Trying to make concurrency work (shared enum)

I am having some issues porting some C++ code. The thing is I have the main thread with 3 other threads. I need a variable shared among them. The main thread should be able to modify it but the other three should not. I'm trying with mutexes etc, but cannot make it work. What I currently have is the following in the Rust playground: Rust Playground

How can I fix it?

Here is a compiling code: Rust Playground

I may be wrong, but it seems like you are confused about what does mutex.lock() does. It returns a handler (think smart pointer) to the protected data, so you need to dereference it to modify:

*state = set_state(State::AcquiringFix).unwrap();

Note the *.

I'm not sure what is the purpose of

let mut state = Arc::get_mut(&mut shared_state).unwrap();

is.

I think I fixed your Arc/Mutex issues, but now there's another problem: Rust Playground

Mmmm the thing is I want to pass that State to the thread functions, but I'm not sure how to do it. With your recomendations, I have done this: Rust Playground, but it gives a timeout :confused: and in my computer it says that cannot borrow `shared_state` as immutable because it is also borrowed as mutable.

About the let mut state = Arc::get_mut(&mut shared_state).unwrap();, it was because I needed the mutable reference to the mutex, to change it, but maybe it's not the way to go. This is what causes the immutable borrow error, but how could I change the state in the Arc from the main thread?

You would lock the mutex, just like in any other thread.

About the let mut state = Arc::get_mut(&mut shared_state).unwrap();, it was because I needed the mutable reference to the mutex, to change it, but maybe it's not the way to go. This is what causes the immutable borrow error, but how could I change the state in the Arc from the main thread?

You do it correctly in line 44.

but it gives a timeout :confused:

Looks like I've maid the main thread to deadlock with itself :sweat_smile: You need to remove line 23 and all is ok.

Take a look here: Rust Playground. It does not timeout and inspects state from a spawned thread.

Great! It's working now, thanks!!!

Nevertheless, I'm trying to put all the code needed for that state change in a macro, and here is what I got:

macro_rules! set_state {
    ($st:expr) => {
        {
            let mut f = try!(fs::File::create(STATE_FILE));
            try!(f.write_all(&format!("{:?}", $st).into_bytes()[..]));

            Ok($st)
        }
    };
    ($st:expr, $shared:ident) => {
        {
            let mut state = $shared.lock().unwrap();

            let mut f = try!(fs::File::create(STATE_FILE));
            try!(f.write_all(&format!("{:?}", $st).into_bytes()[..]));

            *state = $st;
            println!("State changed to {:?}.", *state);

            Ok($st)
        }
    }
}

I have two variants, the first one is for the initial setup, just for doing let shared_state = Arc::new(Mutex::new(set_state!(State::Initializing).unwrap())); while the second one is for actually changing the state. The problem is I get this error:

<std macros>:5:8: 6:42 error: mismatched types:
 expected `()`,
    found `core::result::Result<_, _>`
(expected (),
    found enum `core::result::Result`) [E0308]
<std macros>:5 return $ crate:: result:: Result:: Err (
<std macros>:6 $ crate:: convert:: From:: from ( err ) ) } } )
src/main.rs:31:25: 31:59 note: in this expansion of try! (defined in <std macros>)
src/main.rs:84:44: 84:75 note: in this expansion of set_state! (defined in src/main.rs)
<std macros>:5:8: 6:42 help: run `rustc --explain E0308` to see a detailed explanation

Which I guess is because of the try! / OK() thing, but it's not a problem for me if the macro generates a Result. In fact, it's desirable, since I will be able to check it in the future and decide upon it. I'm new to macros, so I have no clue on how to fix this.

It's not a problem with the macro: you can't use try! inside a function (within a macro or not) that doesn't return a compatible Result<_, E> type. You are, presumably, using it inside main, which does not return a compatible Result type.

Take all the code in main, and put it in a fn try_main() -> Result<(), SomeErrorType>. If you don't know what to use for SomeErrorType, just use Box<::std::error::Error> for the moment.

Hmmm, true, didn't notice. I'm trying to replicate try! doing something like this:

macro_rules! set_state {
    ($st:expr) => {
        {
            let f = fs::File::create(STATE_FILE);
            if f.is_ok() {
                let result = f.unwrap().write_all(&format!("{:?}", $st).into_bytes()[..]);
                if result.is_ok() {
                    Ok($st)
                } else {
                    Err(result.err().unwrap())
                }
            } else {
                Err(f.err().unwrap())
            }
        }
    };
    ($st:expr, $shared:ident) => {
        {
            let mut state = $shared.lock().unwrap();

            let f = fs::File::create(STATE_FILE);
            if f.is_ok() {
                let result = f.unwrap().write_all(&format!("{:?}", $st).into_bytes()[..]);
                if result.is_ok() {
                    *state = $st;
                    info!("State changed to {:?}.", *state);

                    Ok($st)
                } else {
                    Err(result.err().unwrap())
                }
            } else {
                Err(f.err().unwrap())
            }
        }
    }
}

But I get an unused result warning :confused: what could it be?

An unused result warning on what? Without the actual error or code, I have to guess...

...and I guess that you're not doing anything with the result of the set_state! macro? Producing a Result is not sufficient to deal with it; the compiler has absolutely no special support for Result, so if you're not explicitly returning it or checking it, nothing happens, which is when the warning is usually triggered.

Also, it's generally un-idiomatic to use regular if on a Result; a match would be better. I mean, result.err().unwrap() is a huge code smell.

match result {
    Ok(_) => Ok($st),
    Err(e) => Err(e)
}

Although even that can be simplified:

result.map(|_| $st)

That said, this is an aside; you should deal with your more immediate problem first.

Edit: Also, you should use the Reply button on the post you're replying to. If you don't, people don't get notified that you're replying to them and it might take longer to get a response. You can even reply to multiple people by selecting something they said and clicking "quote reply" on the popup.

Thanks! :slight_smile: I wasn't handling the Result in the main() function :sweat_smile: I refactored the code like this, is it the idiomatic way?

macro_rules! set_state {
    ($st:expr) => ({
        let f = fs::File::create(STATE_FILE);
        match f {
            Ok(mut f) => {
                let result = f.write_all(&format!("{:?}", $st).into_bytes()[..]);
                result.map(|_| $st)
            },
            Err(e) => Err(e),
        }
    });
    ($st:expr, $shared:ident) => ({
        let f = fs::File::create(STATE_FILE);
        match f {
            Ok(mut f) => {
                let result = f.write_all(&format!("{:?}", $st).into_bytes()[..]);
                match result {
                    Ok(_) => {
                        let mut state = $shared.lock().unwrap(); // TODO better error handling

                        *state = $st;
                        info!("State changed to {:?}.", $st);

                        Ok($st)
                    },
                    Err(e) => Err(e),
                }
            },
            Err(e) => Err(e),
        }
    });
}

Why do you need a macro here? Can this be done with a function instead?

Macros are a powerful tool, especially suited for sawing off one's leg :slightly_smiling: It is better to avoid or minimize them.

One common pattern with macros is to extract the most functionality into a helper function, and leave the absolute minimum in the macro, like this

fn square_helper(x: i32) {
    x * x
}

macro_rules! square {
  ($e:expr) => ( square_helper($e))
}

Am I correct that $st will be evaluated several times in your macro?

$st should be an enum variable only, it shouldn't need evaluation. Should I change the expr part? About the helper, Yes, probably I can send most of the code to a helper function. What about this?

macro_rules! modify_shared_state {
    ($st:expr, $shared:ident) => ({
        match set_state($st) {
            Err(e) => Err(e),
            Ok(st) => {
                let mut state = $shared.lock().unwrap(); // TODO better error handling

                *state = st;
                info!("State changed to {:?}.", st);

                Ok(st)
            }
        }
    });
}

#[inline]
fn set_state(state: State) -> Result<State, io::Error> {
    let mut f = try!(fs::File::create(STATE_FILE));
    try!(f.write_all(&format!("{:?}", state).into_bytes()[..]));

    Ok(state)
}

$st should be an enum variable only, it shouldn't need evaluation.

If you use $st:expr, then it can be any expression, like {println!("side effect"); State::Foo}.

I still don't understand why would you need a macro here. A function should be enough:

use std::sync::Mutex;
use std::io::prelude::*;
use std::{fs, io};

#[derive(Debug, Clone, Copy)]
enum State {
}

static STATE_FILE: &'static str = "Hello";

fn modify_shared_state(st: State, shared: &Mutex<State>) -> Result<State, io::Error> {
    match set_state(st) {
        Err(e) => Err(e),
        Ok(st) => {
            let mut state = shared.lock().unwrap(); // TODO better error handling

            *state = st;
            Ok(st)
        }
    }
}

#[inline]
fn set_state(state: State) -> Result<State, io::Error> {
    let mut f = try!(fs::File::create(STATE_FILE));
    try!(f.write_all(&format!("{:?}", state).into_bytes()[..]));

    Ok(state)
}

And I'd say that you don't need #[inline] here: When should I use #[inline]? - #3 by huon - guidelines - Rust Internals, especially the last paragraph

OK, thanks! I will do that, and I will use a simple function :slight_smile: