Stream: git-wasmtime

Topic: wasmtime / PR #7056 Use wasi-streams in the wasi-http imp...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2023 at 18:36):

elliottt opened PR #7056 from bytecodealliance:trevor/wasi-http-streams to bytecodealliance:main:

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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

elliottt updated PR #7056.

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

alexcrichton updated PR #7056.

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

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2023 at 23:31):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2023 at 01:01):

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2023 at 16:00):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2023 at 16:06):

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2023 at 23:04):

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt edited PR #7056:

Rework the wasi-http implementation to use wasi-streams, and get the component interface filled out for outgoing requests. Additionally, remove the module shims for wasi-http, as we only intend to support components going forward.
<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 20:06):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 20:23):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 20:47):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 20:49):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 22:42):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2023 at 23:03):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 00:10):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 03:06):

elliottt updated PR #7056.

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

elliottt updated PR #7056.

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

elliottt requested alexcrichton for a review on PR #7056.

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

elliottt has marked PR #7056 as ready for review.

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

elliottt requested wasmtime-core-reviewers for a review on PR #7056.

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

alexcrichton submitted PR review:

Overall looks good to me :+1:

I don't have deep HTTP-specific knowledge so I'm mostly going from WASI/idioms/etc angles. I'm not able to review for example whether this is sufficient for most HTTP-related workloads or such, but I trust y'all on that.

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

alexcrichton submitted PR review:

Overall looks good to me :+1:

I don't have deep HTTP-specific knowledge so I'm mostly going from WASI/idioms/etc angles. I'm not able to review for example whether this is sufficient for most HTTP-related workloads or such, but I trust y'all on that.

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

alexcrichton created PR review comment:

I know that one thing that http clients often do is reuse connections between requests and things like that, so is the thinking here that eventually, in the future, acquiring the TcpStream (or probably some sort of http client) is behind some host-level configuration where host can turn on pooling an stuff like that?

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

alexcrichton created PR review comment:

I'm a little confused on this since stdio for example will return Err(e) here. Similarly for AsyncReadStream it returns Err(e) in a case like this.

Given the comment above though and https://github.com/bytecodealliance/wasmtime/issues/7061 I get the feeling that I'm missing context here though. This all looks correct if https://github.com/bytecodealliance/wasmtime/issues/7061 were solved, so I think I'm mostly curious at this point why stdio/AsyncReadStream are one way but this is a different way

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

alexcrichton created PR review comment:

Is there something easily enough digestable to split out to an issue for this? (I can help work on it)

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

alexcrichton created PR review comment:

Could this be type FieldMap = hyper::HeaderMap or something like that? I suspect that hyper's at-rest implementation is relatively optimized and might be good to reuse if we can.

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

alexcrichton created PR review comment:

Could this have a comment on what the Duration is?

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

alexcrichton created PR review comment:

This is the part of the PR that gives me the most pause. Not necessarily the duplication really but moreso the layers of abstraction. This feels like an extra and not necesarily required layer of buffering that otherwise may not be needed.

So question for you. Reading over some docs I see:

impl<S, D, E> hyper::Body for http_body_util::StreamBody<S>
where
    S: futures_util::Stream<Item = Result<hyper::Frame<D>, E>>,
    D: Buf,

The writer below is more-or-less a simplistic mpsc::Sender<Bytes> where the paried end of mpsc::Receiver<Bytes> seems like it would fit the above signature perfectly. In that sense it seems like impl Body for BodyImpl above this and most of the below could could be deleted in favor of that. One weird part is flushes though but the Body trait doesn't seem to have a flush-method equivalent so there's not much I think that can be done about that.

I'm curious though if y'all dug into this and selected the current strategy due to various characteristics or something like that?

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

alexcrichton created PR review comment:

This looks like it could be async fn ready from my reading, but was there a reason that wasn't chosen?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:00):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:00):

elliottt created PR review comment:

Yep! The plan was to eventually migrate that into a function that we keep in the WasiHttpCtx, but that for now keeping all that logic in the outbound handler would be fine.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:07):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:07):

pchickey created PR review comment:

Fixed by refactoring in 6810c9f86

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:09):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:09):

pchickey created PR review comment:

Yes, our goal is to factor this part out and make it something the embedder provides when building a WasiHttpCtx. However, there is some ambiguity about what the right way to go about that is right now, so we are skipping it in this initial PR.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:22):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:22):

pchickey created PR review comment:

Because this is a oneshot receiver, the receiver gets moved as part of the await in an async fn. The impl future avoids that without having to jump through any weird hoops for cancel safety.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:22):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:22):

pchickey created PR review comment:

explained in 47232f3ce

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:23):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:23):

alexcrichton created PR review comment:

Sounds good! Mostly just wanted to confirm that this was on the radar :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:23):

alexcrichton edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:24):

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:30):

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:32):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:32):

pchickey created PR review comment:

Yes, theres probably a better implementation possible here if we simplify the write budget down to a single frame with some bound. We planned to go back and revisit that at some later point, this was the hack of some code we understood that seemed to work good enough for the moment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:43):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:43):

alexcrichton created PR review comment:

Ah makes sense! I've attempted a refactor using impl Future for &mut T in https://github.com/bytecodealliance/wasmtime/pull/7073 which I think may work to help clean this up a bit

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:46):

elliottt updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:46):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 21:46):

elliottt created PR review comment:

Yep! Done.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:03):

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:04):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:04):

pchickey created PR review comment:

thanks, I never thought of that but of course that is helpful.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:05):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:05):

elliottt created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/7074

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:08):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:08):

pchickey created PR review comment:

returning Err(e) in this method will trap. The stdio and AsyncReadStream places where it causes that trap by receiving an Err from the worker are bugs. We will fix those when we resolve 7061.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:11):

pchickey edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:16):

pchickey updated PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:18):

pchickey edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 22:18):

pchickey requested alexcrichton for a review on PR #7056.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 23:01):

alexcrichton submitted PR review:

:+1:

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2023 at 23:50):

pchickey merged PR #7056.

Last updated: Oct 23 2024 at 20:03 UTC