Remove thread-safe operations in single-threaded contexts

[MarcGuiselin]

Since the client runs in a single thread, I'm not sure why I'm seeing any mutexes and arcs. There shouldn't be any I think. (Please correct me If I'm wrong!)

If the Client should be shared between threads, then at a high level the Client struct should be a wrapper around a Arc<Mutex<InnerClient>> to make the entire underlying implementation thread-safe. But this isn't a good long term solution.

One example of where a mutex is unneeded: When invoking a wrap, a polywrap_wasm::runtime::instance::State is created and stored in a mutex. And then the wrap is run in the same thread. State is accessed several times as wasm calls different import methods. And then State is dropped. At no point between it's creation and deletion will two different threads have access to it.

The compiler might catch this and optimize it in release mode maybe. Not sure. But in any case, removing this sort of code would reduce code complexity and potentially unlock being able to do async multithreading the correct way sometime in the future.

[MarcGuiselin]

I can also see that someone had a really bad time with the borrow checker:

let async_context = Arc::new(Mutex::new(context));

let mut context = async_context.lock().unwrap();
let mutable_context = context.as_mut();
let mut state = mutable_context.data().lock().unwrap();

This is reused several times in packages\wasm\src\runtime\imports.rs.

Note: The Arc::new() can be removed and has no effect.

Creating a mutex, just to unlock it and get a mutable reference to what the borrow checker would otherwise complain about seems like a bit of a hack...

I'm honestly not super experienced with rust, so I'm not sure why this works. I'll ask on a rust forum to see if anyone has any ideas why doing this seems to bypass the compiler errors. I was also super tired yesterday when I was looking at the code so maybe I missed something obvious.

[namesty]

Since the client runs in a single thread, I'm not sure why I'm seeing any mutexes and arcs. There shouldn't be any I think. (Please correct me If I'm wrong!)

If the Client should be shared between threads, then at a high level the Client struct should be a wrapper around a Arc<Mutex<InnerClient>> to make the entire underlying implementation thread-safe. But this isn't a good long term solution.

One example of where a mutex is unneeded: When invoking a wrap, a polywrap_wasm::runtime::instance::State is created and stored in a mutex. And then the wrap is run in the same thread. State is accessed several times as wasm calls different import methods. And then State is dropped. At no point between it's creation and deletion will two different threads have access to it.

The compiler might catch this and optimize it in release mode maybe. Not sure. But in any case, removing this sort of code would reduce code complexity and potentially unlock being able to do async multithreading the correct way sometime in the future.

I can't fully remember all of the technical limitations that led to this; but from what I recall, Wasmer has requirements for its import functions (see: wasmerio/wasmer#3916) and furthermore, for subinvocations we need to use Invoker inside of an import function, hence Invoker needs to also be Send + Sync; thus bubbling up the synchronization mechanisms. I agree there might be ways to abstract this below the consumer-facing API; we could def explore and discuss more on it.

I can also see that someone had a really bad time with the borrow checker:

let async_context = Arc::new(Mutex::new(context));

let mut context = async_context.lock().unwrap();
let mutable_context = context.as_mut();
let mut state = mutable_context.data().lock().unwrap();

This is reused several times in packages\wasm\src\runtime\imports.rs.

Note: The Arc::new() can be removed and has no effect.

Creating a mutex, just to unlock it and get a mutable reference to what the borrow checker would otherwise complain about seems like a bit of a hack...

I'm honestly not super experienced with rust, so I'm not sure why this works. I'll ask on a rust forum to see if anyone has any ideas why doing this seems to bypass the compiler errors. I was also super tired yesterday when I was looking at the code so maybe I missed something obvious.

Yep, this looks wrong, we could simply do:

let mutable_context = context.as_mut();
let mut state = mutable_context.data().lock().unwrap();

And mark context in the import function signature asmut:

move |mut context: FunctionEnvMut<Arc<Mutex<State>>>, values: &[Value]|

Also, this ergonomic has now been released wasmerio/wasmer#3592 maybe it could help simplify other things further.

[MarcGuiselin]

see: wasmerio/wasmer#3916

Ah, that makes sense. I was thinking doing something like this:

unsafe impl Send for State {}
unsafe impl Sync for State {}

I think the above should be safe... but maybe it's not a good idea without first consulting someone who knows wasmer's internals.

And mark context in the import function signature asmut:

move |mut context: FunctionEnvMut<Arc<Mutex<State>>>, values: &[Value]|

I think that's a great idea! In that case, it might be easier to combine a PR resolving this issue with #249

But if possible, I'd like #256 merged first to avoid merge conflicts.

[MarcGuiselin]

I've addressed the build issues and accidental linting with #256