elliottt opened PR #7056 from bytecodealliance:trevor/wasi-http-streams
to bytecodealliance:main
:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #7056.
alexcrichton updated PR #7056.
pchickey updated PR #7056.
elliottt updated PR #7056.
pchickey updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
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:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt updated PR #7056.
elliottt requested alexcrichton for a review on PR #7056.
elliottt has marked PR #7056 as ready for review.
elliottt requested wasmtime-core-reviewers for a review on PR #7056.
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.
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.
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?
alexcrichton created PR review comment:
I'm a little confused on this since stdio for example will return
Err(e)
here. Similarly forAsyncReadStream
it returnsErr(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
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)
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.
alexcrichton created PR review comment:
Could this have a comment on what the
Duration
is?
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 ofmpsc::Receiver<Bytes>
seems like it would fit the above signature perfectly. In that sense it seems likeimpl 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 theBody
trait doesn't seem to have aflush
-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?
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?
elliottt submitted PR review.
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.
pchickey submitted PR review.
pchickey created PR review comment:
Fixed by refactoring in 6810c9f86
pchickey submitted PR review.
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.
pchickey submitted PR review.
pchickey created PR review comment:
Because this is a oneshot receiver, the
receiver
gets moved as part of the await in anasync fn
. The impl future avoids that without having to jump through any weird hoops for cancel safety.
pchickey submitted PR review.
pchickey created PR review comment:
explained in 47232f3ce
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Sounds good! Mostly just wanted to confirm that this was on the radar :+1:
alexcrichton edited PR review comment.
pchickey updated PR #7056.
pchickey updated PR #7056.
pchickey submitted PR review.
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.
alexcrichton submitted PR review.
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
elliottt updated PR #7056.
elliottt submitted PR review.
elliottt created PR review comment:
Yep! Done.
pchickey updated PR #7056.
pchickey submitted PR review.
pchickey created PR review comment:
thanks, I never thought of that but of course that is helpful.
elliottt submitted PR review.
elliottt created PR review comment:
pchickey submitted PR review.
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.
pchickey edited PR review comment.
pchickey updated PR #7056.
pchickey edited PR review comment.
pchickey requested alexcrichton for a review on PR #7056.
alexcrichton submitted PR review:
:+1:
pchickey merged PR #7056.
Last updated: Dec 23 2024 at 12:05 UTC