Stream: git-wasmtime

Topic: wasmtime / PR #11440 feat(p3): implement `wasi:http`

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 17:53):

rvolosatovs opened PR #11440 from rvolosatovs:feat/wasip3-http-impl to bytecodealliance:main:

This PR builds on top of #11221 and will contain the full implementation of wasi:http proposal with refcounting (no child resources). This will be made ready-for-review once all currently ignored tests pass

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 17:53):

rvolosatovs edited PR #11440:

This PR builds on top of #11221 and will contain the full implementation of wasi:http proposal with refcounting (no child resources). This will be made ready-for-review once all currently ignored tests pass

https://github.com/bytecodealliance/wasmtime/pull/11440/commits/480558daacc00f95b594f332b9f1747d15e69f3f is currently the only commit here worth looking at

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 18:08):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 18:11):

rvolosatovs edited PR #11440:

This PR builds on top of #11221 and will contain the full implementation of wasi:http proposal with refcounting (no child resources). This will be made ready-for-review once all currently ignored tests pass

https://github.com/bytecodealliance/wasmtime/pull/11440/commits/4819aade1167a937d6c00f6026eb5451d2f100e3 is currently the only commit here worth looking at

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 18:53):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 18:53):

alexcrichton created PR review comment:

One suggestion perhaps for many of these methods is to have a generic extension trait which supports an arbitrary T for Resource<T> which could help deduplicate these methods?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2025 at 18:53):

alexcrichton created PR review comment:

Could this use trappable_error configuration to aovid the double-layer of Result? That would also enable using ? below to propagate errors.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2025 at 14:19):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2025 at 14:27):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2025 at 14:29):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2025 at 15:04):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 13:53):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 13:55):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:25):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:27):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:31):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:31):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:42):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:51):

rvolosatovs updated PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:55):

rvolosatovs commented on PR #11440:

I'm done for the week and will continue on Monday, but this is pretty close to having all test cases passing.

@alexcrichton @dicej feel free to make the necessary changes in the PR to make the rest of the test cases work if there's a desire to still finish wasi:http this week.
Alternatively, merging this PR as-is and addressing the rest in follow-ups would work as well.

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

rvolosatovs edited a comment on PR #11440:

I'm done for the week and will continue on Monday, but this is pretty close to having all test cases passing.

@alexcrichton @dicej feel free to make the necessary changes in the PR to make the rest of the test cases work if there's a desire to still finish wasi:http this week.
Alternatively, merging this PR as-is and addressing the rest in follow-ups would work as well.

If we're okay with waiting until next week, this should be complete by Monday evening

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2025 at 17:59):

rvolosatovs edited a comment on PR #11440:

I'm done for the week and will continue on Monday, but this is pretty close to having all test cases passing.

@alexcrichton @dicej feel free to make the necessary changes in the PR to make the rest of the test cases work and get this over the line if there's a desire to still finish wasi:http this week.
Alternatively, merging this PR as-is and addressing the rest in follow-ups would work as well.

If we're okay with waiting until next week, this should be complete by Monday evening

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

alexcrichton updated PR #11440.

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

alexcrichton created PR review comment:

Debugging this: in my opinion the bug is that dropping a request resource does not properly close its stream handles. That's not currently possible, however, because during drop there's no access to the store. I made https://github.com/bytecodealliance/wasmtime/pull/11628 to fix this.

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

alexcrichton created PR review comment:

Could you comment this function to the purpose of fut?

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

alexcrichton created PR review comment:

I've debugged these three ignores and it's due to this code needing to happen concurrently with handler::handle now instead of synchronously after. It's easy enough to lift that async block to the join! above except then it runs into the panic described in https://github.com/bytecodealliance/wasmtime/issues/11621 so these will have to wait for that to get fixed

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

alexcrichton created PR review comment:

Could this and get get removed in favor of the Deref impl?

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

alexcrichton created PR review comment:

Mind expanding the docs here to explain what this is used for? For example how headers are immutable when read from some locations but mutable in others. Regardless everything uses an Arc and exhibits copy-on-write behavior.

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

alexcrichton created PR review comment:

This may be preexisting with wasip2 as well, but I'm having a tough time piecing together what's going on here. Mind leaving some comments to what this is doing?

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

alexcrichton created PR review comment:

Could this use a TrappableError from the wasmtime_wasi crate like filesystem & sockets?

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

alexcrichton created PR review comment:

This is one of the reasons I'm seeking more documentation on the various future parameters. This and below looks to be a very careful and particular dance with respect to various errors but I'm not really sure what's going on here in the abstract. I would hope that we can figure out a way to simplify this, but in lieu of that I'm hoping for more docs as to what's going on.

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

alexcrichton created PR review comment:

Is this HttpResult::Ok(( ... ))? still needed? (vs just "do the thing")

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

alexcrichton created PR review comment:

Could you expand the documentation here to include what the various layers of futures are and what their meaning is?

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

alexcrichton created PR review comment:

Technically this is a slightly dangerous ? (and everything below before the body is consumed) because if the body is a guest body the resources won't get properly closed.

Would it make sense to lift the body construction all the way to the top here so that happens first?

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

alexcrichton created PR review comment:

Mind adding a small doc block here explaining what this is doing? The code is pretty self-explanatory but I still think it'd be good to have an intro

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

alexcrichton submitted PR review:

Ok I've gone over this and it looks good to me, thanks again @rvolosatovs!

I've left a bunch of comments below which I'd like to resolve at some point, but doesn't need to block. I'm going to flag this for merge and I'll file follow-ups.

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

alexcrichton created PR review comment:

If you'd like you can drop the for<'a> here and use WasiHttpCtxView<'_> at the end

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

alexcrichton created PR review comment:

I realize this is probably copied from wasip2, but could this log::warn! the error that's discarded here?

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

alexcrichton created PR review comment:

Is it possible to share this with Request::consume_body since it's more-or-less the same function?

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

alexcrichton created PR review comment:

Mind leaving some comments here (and in is_end_stream) about the state machine being managed here? Doesn't need to be too wordy but having headings to grasp on I think would be helpful

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

alexcrichton created PR review comment:

Like response below, could you comment this returned future here and with new?

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

alexcrichton created PR review comment:

Mind leaving some small comments in this function body to help navigate what the state machine is doing? (e.g. a ocmment explaining the signficance of each arm here and there)

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

alexcrichton created PR review comment:

Could you comment function as to what this future is?

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

alexcrichton has marked PR #11440 as ready for review.

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

alexcrichton requested wasmtime-wasi-reviewers for a review on PR #11440.

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

alexcrichton requested dicej for a review on PR #11440.

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

alexcrichton requested wasmtime-core-reviewers for a review on PR #11440.

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

alexcrichton requested wasmtime-default-reviewers for a review on PR #11440.

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

alexcrichton has enabled auto merge for PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 06 2025 at 06:25):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 06 2025 at 06:25):

rvolosatovs created PR review comment:

There's currently no Content-Length header handling, so I would not modify this test until that's implemented in the host.

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

alexcrichton updated PR #11440.

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

alexcrichton has enabled auto merge for PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2025 at 23:08):

alexcrichton merged PR #11440.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:23):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:23):

rvolosatovs created PR review comment:

I forgot to port the test response streaming, which I've done in wasip3-prototyping precisely to fix this. I think it's useful to specifically test the fact that handle returns before response body are streamed, so I'd rather not rearrange the join!.

This is fixed in https://github.com/bytecodealliance/wasmtime/pull/11636

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:23):

rvolosatovs created PR review comment:

Exactly, I've used the feature you've added to fix it in https://github.com/bytecodealliance/wasmtime/pull/11636

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:23):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:25):

rvolosatovs created PR review comment:

I'm not sure that's a good idea for this particular one, since that would require exposing the HttpError to embedders and they'd need to handle the trap case themselves.

While this signature is definitely not ideal, IMO it's clearer and it aligns with what call_handle would otherwise return if called directly

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2025 at 14:25):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 09 2025 at 15:43):

rvolosatovs assigned rvolosatovs to PR #11440.

Last updated: Dec 06 2025 at 07:03 UTC