Stream: git-wasmtime

Topic: wasmtime / issue #6091 Initial implementation of wasi-htt...

Wasmtime GitHub notifications bot (Apr 06 2023 at 04:39):

brendandburns commented on issue #6091:

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.

Wasmtime GitHub notifications bot (Apr 13 2023 at 15:39):

pchickey commented on issue #6091:

Hi, sorry, I haven't forgotten about this PR, I have just been focusing on getting some other stuff done this week and haven't had time to review. I have set aside some time for it tomorrow.

Wasmtime GitHub notifications bot (Apr 15 2023 at 15:06):

brendandburns commented on issue #6091:

@pchickey no worries, I'm not in a rush.

Wasmtime GitHub notifications bot (Apr 20 2023 at 18:08):

pchickey commented on issue #6091:

Can this be rebased on top of https://github.com/bytecodealliance/wasmtime/pull/6228 (or main once it lands) now?

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:51):

brendandburns commented on issue #6091:

Yes, I plan to rebase this once it merges.

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:52):

brendandburns edited a comment on issue #6091:

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.

Wasmtime GitHub notifications bot (Apr 24 2023 at 16:52):

pchickey commented on issue #6091:

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

Wasmtime GitHub notifications bot (Apr 25 2023 at 16:58):

brendandburns commented on issue #6091:

@pchickey this is rebased and ready for initial review.

Wasmtime GitHub notifications bot (Apr 26 2023 at 19:37):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (Apr 28 2023 at 16:31):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (Apr 28 2023 at 18:02):

pchickey commented on issue #6091:

Your "context" struct WasiHttp will need to impl Send in order for Store<WasiHttp> to be used by the async wasmtime apis.

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.

Wasmtime GitHub notifications bot (Apr 28 2023 at 18:14):

pchickey edited a comment on issue #6091:

Your "context" struct WasiHttp will need to impl Send in order for Store<WasiHttp> 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.

Wasmtime GitHub notifications bot (Apr 28 2023 at 19:19):

brendandburns commented on issue #6091:

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.

Wasmtime GitHub notifications bot (Apr 28 2023 at 21:29):

pchickey commented on issue #6091:

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 this into account. We pass 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.

Wasmtime GitHub notifications bot (Apr 28 2023 at 21:32):

pchickey edited a comment on issue #6091:

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.

Wasmtime GitHub notifications bot (May 11 2023 at 01:41):

brendandburns commented on issue #6091:

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.

Wasmtime GitHub notifications bot (May 12 2023 at 20:35):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (May 17 2023 at 13:59):

brendandburns commented on issue #6091:

@pchickey does your :+1: 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 :)

Wasmtime GitHub notifications bot (May 17 2023 at 23:46):

pchickey commented on issue #6091:

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.

Wasmtime GitHub notifications bot (May 18 2023 at 17:36):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (Jun 02 2023 at 02:25):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (Jun 23 2023 at 14:07):

brendandburns commented on issue #6091:

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

Wasmtime GitHub notifications bot (Jun 26 2023 at 19:34):

pchickey commented on issue #6091:

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.

Wasmtime GitHub notifications bot (Sep 05 2023 at 23:23):

brendandburns commented on issue #6091:

Closing as obsolete.

Last updated: Nov 22 2024 at 16:03 UTC