pchickey opened PR #7091 from bytecodealliance:trevor/wasi-http-server
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
-->
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey updated PR #7091.
pchickey has marked PR #7091 as ready for review.
pchickey requested alexcrichton for a review on PR #7091.
pchickey requested wasmtime-core-reviewers for a review on PR #7091.
pchickey requested wasmtime-default-reviewers for a review on PR #7091.
pchickey edited PR #7091:
This follows up #7056 with the remaining pieces used for the wasi-http proxy world.
We made a tweak to the wasi-http wit definition which was missed in the prev PR, but wasn't caught because the incoming request side was still stubbed out.
A new test suite wasi-http-proxy-tests has one trivial test for this new functionality.
A new cli subcommand
wasmtime serve [--addr SOCKADDR] proxy_component.wasm
runs components in the wasi-http proxy world.We make some additional filesystem and cli imports to the proxy world in both the test runner and cli subdommand because the component adapter pulls them in unconditionally. We will work on fixing that in the adapter in the future.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
elliottt updated PR #7091.
alexcrichton submitted PR review:
Looks great to me :+1:
Small comments here and there, please feel free to defer whatever you like to either issues or future PRs.
alexcrichton submitted PR review:
Looks great to me :+1:
Small comments here and there, please feel free to defer whatever you like to either issues or future PRs.
alexcrichton created PR review comment:
While you're at it mind putting
=
constraints on these as well? (works around issues with Cargo's handling of pre-releases)
alexcrichton created PR review comment:
Another possibility here, if you're interested, is to model this as subcommands-as-features. The
wasm-tools
CLI does this where there's a Cargo feature for each subcommand and they're all enabled by default. That way this could be enabled withcargo build --features serve
andserve
would implicitly enable the component model andwasi-http
alexcrichton created PR review comment:
Out of curiosity is it easy enough to have a graceful shutdown here where ctrl-c sets a flag "please exit ASAP"? Otherwise this looks like it cancel all pending work if it's happening during ctrl-c (in which case this probably isn't much better than avoiding installing a ctrl-c in the first place)
alexcrichton created PR review comment:
I think it'd be best if this had similar options to
wasmtime run
, for example--allow-precompiled
,--dir
,--env
, and--profile
. That being said I think it's also totally ok for this to be a follow-up PR too.One thing that I think should also be done is to share the code that configures WASI, e.g. processing many of the
-S
options. For example in theorywasi-nn
and such should all be possible to enable too. That would require some refactoring, however, so perhaps best to open an issue and/or file a follow-up PR for that.
alexcrichton created PR review comment:
This can all be moved outside of the enclosing
async move
I think and theFuture
return could actually be just the receiver (or a boxed receiver, either way)
alexcrichton created PR review comment:
I think this can probably be dropped because the task doesn't return
Err(_)
anywhere
alexcrichton created PR review comment:
I'll leave the decision up to y'all though, I don't feel strongly either way
elliottt submitted PR review.
elliottt created PR review comment:
I like that!
elliottt submitted PR review.
elliottt created PR review comment:
Actually,
wasi-http
already enablescomponent-model
, so we could make this only depend onwasi-http
. However, I don't like that as much because it's not quite as declarative as theserve
command feature.
elliottt submitted PR review.
elliottt created PR review comment:
Yep, agreed. I'll do that in a follow-up PR.
elliottt submitted PR review.
elliottt created PR review comment:
elliottt submitted PR review.
elliottt created PR review comment:
Yeah, I think that should be possible: we could share some state with the running thread that we check for each iteration of the accept loop.
elliottt submitted PR review.
elliottt created PR review comment:
I'm going to punt this to a follow-up PR, in the interest of landing this so that the resourcification work can proceed.
elliottt updated PR #7091.
elliottt submitted PR review.
elliottt created PR review comment:
elliottt submitted PR review.
elliottt created PR review comment:
Great idea! I like that this makes it so much more clear that we spawn the task and return the future that waits on the oneshot channel, rather than returning a future that does both.
elliottt has enabled auto merge for PR #7091.
elliottt merged PR #7091.
Last updated: Dec 23 2024 at 12:05 UTC