Now that #5929 has merged, I think this is ready for review. We may want to merge #6161 first since this PR includes that one.

Yes, I plan to rebase this once #6228 merges.

While we're on the topic of rebase, do you have a good strategy for rebasing this repo since we're doing squash merges? My standard flow is git fetch upstream; git rebase upstream/main but that seems to cause conflicts when rebasing on top of the squashed PR that has merged into main.

I end up using git rebase -i and delete all the lines that correspond to the squashed PRs that already landed.

@pchickey this is rebased and ready for initial review.

@pchickey thanks for the review and the idiomatic suggestions. I will update this PR to reflect the async request above and fix the idiomatic bits too (and add a test)

It will probably take me a few days to get enough free cycles to get it done, but should be by the end of the week.

@pchickey as I dug into this it seems that a bunch of components (e.g. the Store) don't implement the Send trait which means they can't be passed into an async function (at least as far as I can tell)

Do you have a perspective on if it is ok to modify them to implement Send or if there is some other approach that is preferable.

Your "context" struct WasiHttp will need to impl Send in order for Store to be used by the async wasmtime apis. If the struct is composed out of Send elements this should be automatic - if rustc is not letting you have a Send impl, don't derive an unsafe one.

The good news is the design goal for the wasmtime async apis was to work perfectly inside a hyper service, https://github.com/fastly/Viceroy/blob/main/lib/src/execute.rs was the prototype where we actually fleshed all that design out a few years ago. So maybe you can crib some of the patterns there. The bad news is that async Rust has a steep learning curve, and the error messages from rustc aren't quite as brilliant as they are for the rest of the language. If you need to pair program on this for an hour sometime, my calendar next week is pretty open, but I am taking off Friday.

The biggest problem that I see is that in order to make a call into the WASM module you need to pass the Linker and the Store into the async function and neither of those implement Send

The reason that I need to pass it is because the only time that you can get access to that is when the context is constructed (at least as far as I can tell). If there was a way to statically get at the Linker and the Store then it would just be a question of making my context Send but those other wasmtime resources need to be part of the context also.

Looks like you worked around this by constructing the store and linking inside each HTTP execution:

https://github.com/fastly/Viceroy/blob/main/lib/src/execute.rs#L301

Which might be possible here, but would require some pretty significant rewiring since that is created/configured via flags etc in a different place.

That's right - the pattern required is to construct a store inside each execution so we can give the host functions a mut ref to the Store's T, otherwise you would need to use Arc<Mutex<>> and lock every use to get something Send and Sync to communicate with outside of the execution. The context will have to be designed to take into account that it is created from the embedder's per-http request execution, not the other way around.

Viceroy passes in the LinkerPre structure to amortize linking, but that will be up to the embedder. Store::new and LinkerPre.instantiate_async is designed to be extremely fast when used with the memory pooling features in the Engine.

Just a quick update that this PR is proving to be more thorny/invasive than I thought b/c of the interplay of Store/Linker/Host.

I think that there needs to be some significant refactoring of how the run command is written to make parts of it accessible in other parts of the code-base.

I'm still working on it (slowly) but it might be a bit of time before this arrives.

@pchickey ok, I got this working with the switch to call_async and instantiate_async.

Can you take a look and see if this is what you had in mind?

If it looks directionally correct, I'll work on the style and tests.

Thanks!

@pchickey does your 👍 mean that this looks correct? Or did it just mean "acknowledged, I will take a look when I get a chance"?

Sorry, the meaning of response emojis are sometimes hard to parse :)

Hi, sorry, the +1 was an acknowledgement that I was going to review it, but I haven't had time to spend on it yet, this week I have been heads down trying to move all of the preview2-prototying repo into this one.

Taking a brief look now, this didn't turn out exactly like I had envisioned, but I am not sure exactly what to suggest to change, I will have to play with refactoring it a bit myself to figure out the right direction. Sorry that isn't very actionable feedback. I will spend time on this next week.

@pchickey sounds good. I actually spent a bunch of time on the refactoring also, I took three different paths at it and this was the one that seemed most "right"

But I'm definitely open to other options.

The most radical one would be to make all wasmtime executions async instead of the current sync default, but I considered that to be to significant of a change to entertain.

@pchickey this is updated with a rebase to reflect the recent wit changes.

@pchickey friendly ping on this, I suspect that you are busy on other things, so no rush, but wondering what the status of this is.

Thanks!

Right now I am working on getting the preview 2 host streams and pollable implementation landed in #6556. That is the prerequisite for getting wasi-http integrated with the rest of the component model implementation in wasmtime.

Once #6556 lands we need to do pretty hefty renovations to the wasi-http implementation to rebase it on top of that work. @dicej has successfully made a new implementation based on a somewhat similar implementation of streams and pollable https://github.com/fermyon/spin/blob/132a32daed6632768d52a9336125073e87fee1e8/crates/wasi-cloud/src/http.rs. The current consensus is that Joel will spearhead the effort to get wasmtime-wasi-http running on top of streams - we are expecting this to more or less amount to a rewrite as he ports his work from the spin repo into this crate. His upstream work also contains a (related, but again a little bit different) implementation of the server piece of this puzzle.

So, I think at this point we have to set this PR aside.

Closing as obsolete.