Stream: git-wasmtime

Topic: wasmtime / issue #6195 feat: align wasi-http with compone...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 15:55):

eduardomourar commented on issue #6195:

@pchickey and @brendandburns , could you review this PR, please? Ideally, we should merge this prior to https://github.com/bytecodealliance/wasmtime/pull/6091.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 17:04):

brendandburns commented on issue #6195:

I think it is probably worth splitting this into two PRs, one containing all of the common implementations and one with the refactors to the HTTP implementation.

Using the common code for things like streams seems reasonable.

I don't really see the point of the HTTP refactors, they seem to add a bunch of complexity without really reducing the amount of code that is required.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 17:51):

eduardomourar commented on issue #6195:

I don't really see the point of the HTTP refactors, they seem to add a bunch of complexity without really reducing the amount of code that is required.

This is important to integrate with preview2, but also to have less code change whenever the resources will be auto-generated by wit-bindgen. We will be a step further in that direction instead of diverging more and more from the expected pattern.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 17:56):

pchickey commented on issue #6195:

This looks to me like a reasonable path to align with some of the changes coming down the pipe from preview2-prototyping. It doesn't make those changes totally drop-in, but using the streams and table abstraction does take us part of the way there.

I agree that, without that context, it looks like added complexity without reducing the amount of code required.

So really the question is whether adding this now is appropriate, or if we try to land Brendan's other patches first. Unfortunately I am still deep in the preview 2 filesystem work over in p2-p and I haven't context switched back to HTTP to review Brendan's PRs yet. https://github.com/bytecodealliance/wasmtime/pull/6228 is pretty straightforward but https://github.com/bytecodealliance/wasmtime/pull/6091 has been sitting for longer and I haven't dug into it yet.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 18:27):

alexcrichton commented on issue #6195:

I'm going to list @pchickey as the reviewer for this since I think he's got this in his wheelhouse

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 19:46):

eduardomourar commented on issue #6195:

@pchickey , I can understand if we want to first merge the other PRs. After they are, I will integrate the changes back here then.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 19:44):

brendandburns commented on issue #6195:

Just for my own understanding, I don't think that anything in http_impl.rs changes with the move to preview 2 does it? That struct already implements the Host trait that is defined by the wasi-http with bindings. Why does switching to Table make it more compatible? I think I'm missing something.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 20:37):

eduardomourar commented on issue #6195:

If you look into this example here and other parts of the host implementation, you will see that the table abstraction is being assumed to be available and it is the default place to store the id for each handle and associated resource instance. By doing the same for the HTTP module in this PR, we would only hold specific data in its context (right now there is none) and rely on the common context instead. This simplification is not that evident here because I copied the shared code to avoid creating a dependency to the preview2-prototyping repo.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 20:43):

eduardomourar edited a comment on issue #6195:

I would expect the http_impl.rs to be modified, yes. If you look into this example here and other parts of the host implementation, you will see that the table abstraction is being assumed to be available and it is the default place to store the id for each handle and associated resource instance. By doing the same for the HTTP module in this PR, we would only hold specific data in its context (right now there is none) and rely on the common context instead. This simplification is not that evident here because I copied the shared code to avoid creating a dependency to the preview2-prototyping repo.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 21:17):

pchickey commented on issue #6195:

Another wrinkle is that we'll be spreading this implementation across multiple crates. wasi-common will define the Table, a trait WasiInputStream , and then hook up all the input-stream methods in the io.streams wit to lookup in the table and downcast it to a Box<dyn WasiInputStream>, and then invoke the method.

wasmtime-wasi-http's role will be to define struct HttpBodyInputStream( {whatever the right tokio/hyper primitives here} ), impl wasi_common::WasiInputStream for HttpBodyInputStream, and then creation of a new stream will be to cast it to a Box<dyn WasiInputStream> and stick it into a table, which is shared with the wasi_common::WasiCtx.

We may end up needing to pull some additional tricks to get wasi-common and wasmtime-wasi-http to each access the table via &mut Table instead of using an Arc<Mutex<Table>> to handle sharing, but that is just an optimization we can do once the basics work.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 21:17):

pchickey edited a comment on issue #6195:

Another wrinkle is that we'll be spreading this implementation across multiple crates. wasi-common will define the Table, a trait WasiInputStream , and then hook up all the input-stream methods in the io.streams wit to lookup in the table and downcast it to a Box<dyn WasiInputStream>, and then invoke the method.

wasmtime-wasi-http's role will be to define struct HttpBodyInputStream( {whatever the right tokio/hyper primitives here} ), impl wasi_common::WasiInputStream for HttpBodyInputStream, and then creation of a new stream will be to cast it to a Box<dyn WasiInputStream> and stick it into a table, which is shared with the wasi_common::WasiCtx.

We may end up needing to pull some additional tricks to get wasi-common and wasmtime-wasi-http to each access the table via &mut Table instead of using an Arc<Mutex<Table>> to handle sharing, but that is just an optimization we can do once the basics work.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2023 at 22:33):

brendandburns commented on issue #6195:

It seems like much of the refactor is unrelated to streams, I get that consistent stream handling makes sense, but doesn't seem like using a Table to store the request/response objects is inherently better than having a struct with multiple hashtables inside of it. Indeed it seems to complicate the implementation instead.

Moreover, given that there are multiple other wasi-modules in wasmtime that are even further from being component-linker compatible, I'd rather we focused energy on getting the HTTP implementation correct in terms of user-facing surface area and wait for preview2 to arrive in wasmtime before we worry about these kind of changes.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 00:05):

eduardomourar commented on issue #6195:

The problem is that currently we cannot use a Wasm component (+ preview2) in different runtimes (in my case wasmtime, Node.js and browser). I guess other people/companies are facing the same issue.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 00:05):

eduardomourar edited a comment on issue #6195:

The problem is that currently we cannot use a Wasm component (+ preview2) in different runtimes (in my case wasmtime, Node.js and browser). I guess other people/companies are facing the same issue.

The way I see, we have two options:

I actually preferred the second option, which was the one decided originally. Although, you can take this PR as a compromise where we will temporarily maintain those in two places in order to bring the learnings upstream.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 18:54):

eduardomourar edited a comment on issue #6195:

The problem is that currently we cannot use a Wasm component (+ preview2) in different runtimes (in my case wasmtime, Node.js and browser). I guess other people/companies are facing the same issue.

The way I see, we have two options:

I actually preferred the second option, which was the one decided originally. Although, you can take this PR as a compromise where we will temporarily maintain those in two places in order to bring the learnings upstream and they will eventually converge.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2023 at 22:10):

pchickey commented on issue #6195:

@eduardomourar We're eager to get this landed and then start working on the next steps incrementally. Can you please get this passing based on main, i.e. ignoring https://github.com/bytecodealliance/wasmtime/pull/6836 for now? Alex has limited availability this week, and we don't know precisely when he'll be able to get that fixed up and landed,

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2023 at 22:10):

pchickey edited a comment on issue #6195:

@eduardomourar We're eager to get this landed and then start working on the next steps incrementally. Can you please get this passing based on main, i.e. ignoring https://github.com/bytecodealliance/wasmtime/pull/6836 for now? Alex has limited availability this week, and we don't know precisely when he'll be able to get that fixed up and landed, so we don't want to wait for it.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 01:22):

pchickey commented on issue #6195:

Sorry to hijack your git history @eduardomourar but I want to get this landed while you are asleep!


Last updated: Dec 23 2024 at 13:07 UTC