Stream: git-wasmtime

Topic: wasmtime / Issue #793 Reconciling `wasmtime::Instance` an...

Wasmtime GitHub notifications bot (Jun 06 2020 at 20:58):

autodidaddict commented on Issue #793:

I'm running into problems right now in a few downstream crates that I'm writing on top of wasmtime where I am having to jump through all kinds of hoops and create some pretty gnarly looking code because the wasmtime Instance isn't send. I can't even hide the instance under an Arc<RwLock<...>> because of its internal use of Rc<RefCell>>s.

Wasmtime GitHub notifications bot (Sep 15 2020 at 06:39):

Waridley commented on Issue #793:

How does https://github.com/bytecodealliance/wasmtime/pull/1363 affect this?

Wasmtime GitHub notifications bot (Sep 15 2020 at 08:35):

Waridley commented on Issue #793:

It seems the most up-to-date explanation of the problem here may be at https://github.com/bytecodealliance/wasmtime/issues/777#issuecomment-625942214
I still really hope it's possible to make Instance be Send. This issue causes me to need to decide whether to use Wasmer instead (speaking of which, how do they do it? Is it actually safe? What's different? I actually thought the way wasmer Modules need a Store made using threads harder until I tried it with wasmtime and discovered this issue.) Or just wrap it in a newtype and unsafe impl Send for it... But then I'll have no idea if it's actually working right until something breaks.

Wasmtime GitHub notifications bot (Sep 15 2020 at 14:09):

alexcrichton commented on Issue #793:

At this point Instance is pretty unlikely to ever be Send. It is, however, safe to move everything connected to a Store to a different thread all at once. It's just not safe to use a Store simultaneously across threads. We haven't documented this, however, it's just a feature of the implementation that will likely always be true. I don't know how wasmer internals work, but for Wasmtime it is not memory safe to add unsafe impl Send for Instance.

Wasmtime GitHub notifications bot (Sep 15 2020 at 15:05):

Waridley commented on Issue #793:

So basically there's no way to enforce in the type system that you move everything connected to the Store all at once?

Wasmtime GitHub notifications bot (Sep 16 2020 at 00:52):

alexcrichton commented on Issue #793:

Correct, yeah, we haven't implemented a way of doing that yet. (I'm not sure if it exists for us)

Wasmtime GitHub notifications bot (Sep 17 2020 at 17:31):

Waridley commented on Issue #793:

Thanks. Turns out Wasmer just has an unsafe impl Send for InstanceHandle {} https://github.com/wasmerio/wasmer/blob/a2e744aba6c4898841f8a67f4d56a24f204ee439/lib/vm/src/instance.rs#L769-L772

And a comment saying /// TODO: this needs extra review. Not to call anyone out, but I'd rather use a library that has thoroughly considered all safety implications and decided that it's not sound to add this impl, rather than one that just assumes it's fine... I can just wrap it in a newtype and impl Send on that myself, and at least I'll have a better understanding of why it's unsafe and what invariants I need to uphold to make it sound.

Wasmtime GitHub notifications bot (Sep 17 2020 at 18:50):

bjorn3 commented on Issue #793:

It isn't safe. There is a Clone impl that shares the memory. This means that you can use send a clone to another thread and then call table_set from multiple threads at the same time. There doesn't seem to be any synchronization, so this is a race-condition and thus UB.

Wasmtime GitHub notifications bot (Sep 17 2020 at 18:57):

bjorn3 edited a comment on Issue #793:

It isn't safe. There is a Clone impl that shares the memory. This means that you can use send a clone to another thread and then call table_set from multiple threads at the same time. There doesn't seem to be any synchronization, so this is a race-condition and thus UB.

Edit: missed that .set has a lock.

Wasmtime GitHub notifications bot (Sep 17 2020 at 19:01):

bjorn3 edited a comment on Issue #793:

It isn't safe. There is a Clone impl that shares the memory. This means that you can use send a clone to another thread and then call table_set from multiple threads at the same time. There doesn't seem to be any synchronization, so this is a race-condition and thus UB.

Edit: missed that .set has a lock.
Edit2: host_state contains Box<dyn Any>, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must be Box<dyn Any + Sync>.

Wasmtime GitHub notifications bot (Sep 17 2020 at 19:05):

bjorn3 edited a comment on Issue #793:

It isn't safe. There is a Clone impl that shares the memory. This means that you can use send a clone to another thread and then call table_set from multiple threads at the same time. There doesn't seem to be any synchronization, so this is a race-condition and thus UB.

Edit: missed that .set has a lock.
Edit2: host_state contains Box<dyn Any>, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must be Box<dyn Any + Sync>.
Edit3: reported bug to wasmer

Wasmtime GitHub notifications bot (Sep 17 2020 at 19:06):

bjorn3 edited a comment on Issue #793:

It isn't safe. There is a Clone impl that shares the memory. This means that you can use send a clone to another thread and then call table_set from multiple threads at the same time. There doesn't seem to be any synchronization, so this is a race-condition and thus UB.

Edit: missed that .set has a lock.
Edit2: host_state contains Box<dyn Any>, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must be Box<dyn Any + Sync>.
Edit3: reported bug on the wasmer repo

Wasmtime GitHub notifications bot (Apr 07 2021 at 17:20):

Knight-X commented on Issue #793:

Can we run the wasi example on the async pattern now ? It looks like we just have async Func and Store, but we do not have async Linker. If we can, is there any tutorial to run wasi example on async mode?

Wasmtime GitHub notifications bot (Apr 07 2021 at 21:34):

alexcrichton closed Issue #793:

Currently as of today the Instance type in the wasmtime crate is not Send. This means that you cannot send it to other threads once it's created. There's a whole bunch of technical issues as to why this is, but for the sake of this issue I'd like to assume a world where we can instantly design everything the way we want.

In our eventual world, for example, Module is Send and Sync along with most other types in wasmtime. We, in this world, get to decide whether we want Instance to be Send or not. As a quick note we fundamentally cannot make Instance Sync because that would allow concurently invoking the same function and (at least) concurrently modifying globals in a non-atomic fashion. This is what we've historically discussed, where Send is desired to send instances to other threads.

I want to write down some reasoning, though, for why I think this may actually be extremely difficult if not impossible. There's two issues I see with trying to make Instance a send-able type:

• One is that all the externals today rely on reference counting. Once you put Rc somewhere it's fundamentally neither Send nor Sync. Types like Global, however, internally contain an Rc. We may be able to fix this by only allowing acquisition of &Global from Instance, however. This is the easier of the constraints to solve.

• Perhaps the more fundamental constraint though is how we want to deal with function imports. Today this is done with the Callable trait, and those values are stored within an Instance (transitively through Func and such). The Callable, trait, however, does not require Send, which means that, again, Instance is not send. For the rest of this issue I'll discuss this problem.

One possible solution to the latter point would be to add Send as a requirement to the Callable trait, but this also unfortunately is not a great API decision. That requires everyone, even those who don't need it, to implement the Send trait. That's surprisingly restrictive because there are legitimate cases where you don't want to implement Send or deal with the overhead.

The only real solution I know of is to somehow get generics into the picture. For example we can make Instance<T> conditionally Send based on T, allowing users to enable an instance to be Send if they configure it accordingly, but also accomodating users who don't want to deal with Send and only want to work on one thread.

This "real solution", though, doesn't really make sense to me. What is T in Instance<T>? Naively it's basically "the import object" but how can we express this? Is there a way we can create a trait to represent this?

---

The tl;dr; of this issue is basically:

• I assert that it is a local maximum today (and likely beyond) for Instance to not be Send.
• For Instance to be Send, I think it will require a type parameter, like Instance<T> where T is the "import object" with some trait to make it work. I haven't the foggiest though how this trait would be designed that satisfies all our constraints for possible optimizations and such.

As a result, I'd like to propose that we commit to, for now, the Instance type not being Send, and then getting as much mileage out of that as we can.

Wasmtime GitHub notifications bot (Apr 07 2021 at 21:35):

alexcrichton commented on Issue #793:

@Knight-X currently wasi-common doesn't have async bindings, but that is planned for the future. Using a Linker you can insert Func values created from async host functions too.

Last updated: Oct 23 2024 at 20:03 UTC