Wasmtime GitHub notifications bot (Apr 18 2023 at 15:55): 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.
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.
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.
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.
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
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.
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.rschanges with the move to preview 2 does it? That struct already implements theHosttrait that is defined by thewasi-httpwith bindings. Why does switching to Table make it more compatible? I think I'm missing something.
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.
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.
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-commonwill define theTable, atrait WasiInputStream, and then hook up all theinput-streammethods in theio.streamswit to lookup in the table and downcast it to aBox<dyn WasiInputStream>, and then invoke the method.
wasmtime-wasi-http's role will be to definestruct 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 thewasi_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.
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-commonwill define theTable, atrait WasiInputStream, and then hook up all theinput-streammethods in theio.streamswit to lookup in the table and downcast it to aBox<dyn WasiInputStream>, and then invoke the method.
wasmtime-wasi-http's role will be to definestruct 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 thewasi_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 Tableinstead of using anArc<Mutex<Table>>to handle sharing, but that is just an optimization we can do once the basics work.
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
Tableto 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.
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.
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:
- we go forward with this PR (after the pending PRs are merged, obviously) by bringing preview2 partially here;
- or we move all this logic to the preview2-prototyping repo;
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.
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:
- we go forward with this PR (after the pending PRs are merged, obviously) by bringing preview2 partially here;
- or we move all this logic to the preview2-prototyping repo;
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.
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,
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.
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: Nov 22 2024 at 16:03 UTC