Reconciling wasmtime::Instance and Send

[alexcrichton]

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.

[sfackler]

I would very much like Instance to be Send. I don't necessarily see myself shipping an Instance explicitly between threads, but rather working with interfaces that expect things to be Send (e.g. a multi-threaded async runtime).

On the Rc front, it seems like that can be pretty trivially solved by just switching to Arc, right?

It seems like it'd be possible through some kind of marker trait design to parameterize over Send-ness. Particularly if it's an opaque, sealed trait it should allow the implementation to remain flexible - the associated types and methods on the trait can vary as the implementation wants.

[alexcrichton]

Yes it should be simple enough to switch Rc values to Arc (there's quite a few to go through but it's not the end of the world). The more fundamental problem is what to do about the Func type. I don't think we can acceptable say "Func must be Send" because that excludes the usage of Rc entirely, but similarly if we say Func can never be Send then neither can Instance.

I'm not sure I understand though what you mean by a marker trait, which presumably is targeted at this problem, can you elaborate what you're thinkign?

[sfackler]

Sure - roughly something like this:

pub trait Style: Sealed {
    // some public API associated types
    type Func;

    // but mostly hidden implementation-defined glue code
}

pub enum LocalStyle {}

impl Style for LocalStyle {
    type Func = NonSendFunc;

    // ...
}

pub enum SendStyle {}

impl Style for SendStyle {
    type Func = SendFunc;

    // ...
}

pub struct Instance<S>
where
    S: Style,
{
    // ....
}

So the idea is that the Style parameter contains all of the types/functionality that would change depending on if the instance's environment is Send or not. Since it's a sealed trait you can maintain flexibility on the implementation side by adding and removing "private" APIs within the trait without worrying about breaking downstream impls since those can't exist.

[alexcrichton]

Ah thanks, makes sense. I feel though that it would be pretty difficult to compose crates if that were used. If only one crate at a time used wasmtime (which may be the case?) then it would be ok but otherwise as a producer of a *Func, for example, I've got to pick one and I may wish to also be somewhat generic and/or abstract.

That being said it's definitely a more concrete suggestion than my own, so it's likely a good starting point!

[sfackler]

I think you should be able to define some kind of inheritance (or at least conversion) to go from a SendFunc to a NonSendFunc, but yeah, the details of the API would need to be worked out for sure.

[fitzgen]

Another option is to require funcs be send but provide some kind of escape hatches like proxy-access-to-the-original-thread and/or make-call-fallible-and-error-out-dynamically-if-we're-on-the-wrong-thread.

[autodidaddict]

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.

[Waridley]

How does #1363 affect this?

[Waridley]

It seems the most up-to-date explanation of the problem here may be at #777 (comment)
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.

[alexcrichton]

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.

[Waridley]

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

[alexcrichton]

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

[Waridley]

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.

[bjorn3]

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

[Knight-X]

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]

@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.