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 ofRc<RefCell>>
s.
Waridley commented on Issue #793:
How does https://github.com/bytecodealliance/wasmtime/pull/1363 affect this?
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 makeInstance
beSend
. This issue causes me to need to decide whether to useWasmer
instead (speaking of which, how do they do it? Is it actually safe? What's different? I actually thought the way wasmerModule
s need aStore
made using threads harder until I tried it with wasmtime and discovered this issue.) Or just wrap it in a newtype andunsafe impl Send
for it... But then I'll have no idea if it's actually working right until something breaks.
alexcrichton commented on Issue #793:
At this point
Instance
is pretty unlikely to ever beSend
. It is, however, safe to move everything connected to aStore
to a different thread all at once. It's just not safe to use aStore
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 addunsafe impl Send for Instance
.
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?
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)
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-L772And 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.
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 calltable_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.
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 calltable_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.
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 calltable_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
containsBox<dyn Any>
, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must beBox<dyn Any + Sync>
.
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 calltable_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
containsBox<dyn Any>
, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must beBox<dyn Any + Sync>
.
Edit3: reported bug to wasmer
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 calltable_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
containsBox<dyn Any>
, but it can be accessed from multiple threads at the same time through the aforementioned way. This means that it must beBox<dyn Any + Sync>
.
Edit3: reported bug on the wasmer repo
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?
alexcrichton closed Issue #793:
Currently as of today the
Instance
type in thewasmtime
crate is notSend
. 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
isSend
andSync
along with most other types inwasmtime
. We, in this world, get to decide whether we wantInstance
to beSend
or not. As a quick note we fundamentally cannot makeInstance
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, whereSend
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 neitherSend
norSync
. Types likeGlobal
, however, internally contain anRc
. We may be able to fix this by only allowing acquisition of&Global
fromInstance
, 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 anInstance
(transitively throughFunc
and such). TheCallable
, trait, however, does not requireSend
, 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 theCallable
trait, but this also unfortunately is not a great API decision. That requires everyone, even those who don't need it, to implement theSend
trait. That's surprisingly restrictive because there are legitimate cases where you don't want to implementSend
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>
conditionallySend
based onT
, allowing users to enable an instance to beSend
if they configure it accordingly, but also accomodating users who don't want to deal withSend
and only want to work on one thread.This "real solution", though, doesn't really make sense to me. What is
T
inInstance<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 beSend
.- For
Instance
to beSend
, I think it will require a type parameter, likeInstance<T>
whereT
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 beingSend
, and then getting as much mileage out of that as we can.
alexcrichton commented on Issue #793:
@Knight-X currently
wasi-common
doesn't have async bindings, but that is planned for the future. Using aLinker
you can insertFunc
values created from async host functions too.
Last updated: Dec 23 2024 at 13:07 UTC