Skip to content

feat: implement wasi:http #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 19 commits into from
Apr 7, 2025
Merged

feat: implement wasi:http #58

merged 19 commits into from
Apr 7, 2025

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Mar 4, 2025 •
edited
Loading

Implementation of WebAssembly/wasi-http#162

With Wasmtime binary built from this PR with this patch applied: b007214 (from #60), I have the complete round trip working using components in https://github.com/rvolosatovs/p3-http-test

@rvolosatovs rvolosatovs force-pushed the feat/http branch 3 times, most recently from f038d56 to d1f5188 Compare March 5, 2025 14:29
@rvolosatovs rvolosatovs force-pushed the feat/http branch 2 times, most recently from 29f307d to 90b9354 Compare March 12, 2025 16:42
@rvolosatovs rvolosatovs mentioned this pull request Mar 21, 2025
Merged
@rvolosatovs rvolosatovs force-pushed the feat/http branch 8 times, most recently from ea0f439 to c62ba73 Compare March 28, 2025 18:03
@alexcrichton alexcrichton mentioned this pull request Apr 1, 2025
Merged
@rvolosatovs
Copy link
Member Author

rvolosatovs commented Apr 1, 2025 •
edited
Loading

cc @pchickey as we just discussed, pushed 926d272, i.e. not porting:

@rvolosatovs rvolosatovs force-pushed the feat/http branch 2 times, most recently from 782fce1 to 926d272 Compare April 1, 2025 18:32
@dicej dicej mentioned this pull request Apr 2, 2025
Closed
@rvolosatovs
Copy link
Member Author

Added Content-Length handling for outgoing requests and responses in feb9d10, unlike wasip2, we do need to attempt to transmit the request/response to catch the error due to the lack of OutgoingBody::finish.
Since there is no way to report an error on construction of request/response, instead I enforce the fact that Content-Length header set by guests is well-formed.

None of this validation is enforced on requests/responses originating in the host (default HTTP client will enforce that incoming response body is compliant with Content-Length, testing that behavior with Hyper is still TODO) - it is assumed that it is embedders responsibility to ensure that http::Requests they feed to components have well-formed Content-Length header values and their bodies respect that. More concretely, in case of Wasmtime CLI, https://docs.rs/hyper/latest/hyper/index.html is expected to handle Content-Length correctly. From what I can see, that aligns with the existing p2 impl.

cc @pchickey

@rvolosatovs rvolosatovs force-pushed the feat/http branch 3 times, most recently from b23067a to 9b2b834 Compare April 4, 2025 15:15
@rvolosatovs rvolosatovs marked this pull request as ready for review April 4, 2025 15:52
@rvolosatovs rvolosatovs added this pull request to the merge queue Apr 4, 2025
rvolosatovs and others added 18 commits April 7, 2025 13:51
@rvolosatovs
feat(p3): implement wasi:http
46ec551 
Signed-off-by: Roman Volosatovs <[email protected]>
@dicej @rvolosatovs
fix rebase breakage
e35bb4b 
Signed-off-by: Joel Dice <[email protected]>
@dicej @rvolosatovs
drop Watch<StreamWriter<T>>::into_inner in wasi-http
76bf162 
Dropping the `Watch` itself just decrements the reference count, which won't
actually drop the `StreamWriter` until the promise completes.  The result was
that some of the HTTP tests were hanging.  This was my mistake, sorry!

Signed-off-by: Joel Dice <[email protected]>
@rvolosatovs
http: do not strip incoming request headers
2c4b9cb 
As discussed with @pchickey

See also
bytecodealliance/wasmtime#7538 (comment) and following discussion

Signed-off-by: Roman Volosatovs <[email protected]>
@alexcrichton @rvolosatovs
Update tests with new wit-bindgen APIs
4090f01
@rvolosatovs
feat(p3/http): handle content-length
e77002c 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
chore: update to new API
2e65763 
Signed-off-by: Roman Volosatovs <[email protected]>
@dicej @rvolosatovs
fix more rebase damage; assert read buffers have non-zero capacity
bfbb830 
This should make #100 more debuggable by turning it in to an assertion failure
in `OutgoingRequestBody::poll_frame`.  It seems to be doing a zero-length read
(presumably by accident), but I haven't traced it further than that, yet.

Signed-off-by: Joel Dice <[email protected]>
@rvolosatovs
fix: reserve buffer capacity after reads
022978d 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
feat(p3/http): implement serving
3a25bb0 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
feat(p3/http): implement wasmtime serve
045ef9e 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
feat(p3/http): link p3 http in run
c5bd4c8 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
fix(p3/http): always set "path and query"
ae37836 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
fix: feature-gate CLI HTTP support
50528df 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
avoid setting a default path and query
bd2d948 
Personally, I think defaulting to `/` if no path with query is set would
be the expected behavior, but wasip2 contained tests explicitly testing
against that behavior, so I disagree and commit

Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
fix tracing import
85f8836 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
http: enable TLS on riscv64
d9865b3 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
cli/serve: integrate shutdown from upstream
9a4a0ae 
Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs rvolosatovs enabled auto-merge April 7, 2025 12:01
@rvolosatovs rvolosatovs added this pull request to the merge queue Apr 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 7, 2025
@rvolosatovs rvolosatovs enabled auto-merge April 7, 2025 12:34
@rvolosatovs
ignore failing riscv64 tests
5e8f880 
#105

Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs rvolosatovs added this pull request to the merge queue Apr 7, 2025
Merged via the queue into main with commit e510b3f Apr 7, 2025
44 checks passed
@rvolosatovs rvolosatovs mentioned this pull request Apr 7, 2025
Draft
@alexcrichton alexcrichton deleted the feat/http branch April 15, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rvolosatovs @dicej @alexcrichton