Skip to content
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.

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

experimental wasi-http support #1553

Closed
wants to merge 2 commits into from
Closed

experimental wasi-http support #1553

wants to merge 2 commits into from

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Jun 2, 2023

This adds experimental support for the wasi-http proposal. Note that the WIT files have been copied from that repo and modified to avoid name conflicts. Eventually, we'll want to use wit-deps to manage them.

The good:

  • Asynchronous, concurrent streaming supported for both inbound and outbound requests and responses
  • Includes an example that uses a custom async executor which could evolve into an async SDK API

The bad:

  • Outbound request/response trailers not yet supported since reqwest does not yet support them
  • Some less-used stream operations not yet implemented (e.g. write_zeros), but should be easy to add

The ugly:

  • Had to rename a bunch of interfaces (e.g. poll -> poll2) to avoid conflicts with our snapshot of preview2-protyping
  • We've commented-out the clock, random, and stdio parts of the world in favor of ambient WASI Preview 1 support, again due to the aforementioned conflicts
  • Implementing wasi-cloud as a host component and passing a handle to it to the wasi-http trigger is awkward
  • The host component currently traps on certain runtime errors because the WIT interface is not fallible but probably should be
  • The WIT spec is not clear on what should happen in certain corner cases (e.g. if you mutate headers after you've passed them to a constructor, or if you send the same request more than once)

Long term, we'd like to contribute the Spin-agnostic parts of this implementation upstream to the Bytecode Alliance. For now, we'll let it incubate here in Spin.

@dicej dicej force-pushed the wasi-http branch 3 times, most recently from 7a2e8ad to 4e0207b Compare June 6, 2023 03:16
@dicej dicej marked this pull request as ready for review June 6, 2023 14:33
@@ -135,10 +138,11 @@ impl<Executor: TriggerExecutor> TriggerExecutorBuilder<Executor> {
&mut builder,
spin_config::ConfigHostComponent::new(runtime_config.config_providers()),
)?;
}
Some(builder.add_host_component(wasi_cloud::WasiCloudComponent)?)
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #1561 + TriggerExecutor::configure_engine should allow some of this to be simplified.

Comment on lines +176 to +171
lint-examples:
name: Lint Examples
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we try to break these changes out to a separate PR? Seems useful and it would be a shame to block these changes on landing wasi-http support (which isn't really related).

crates/common/Cargo.toml Outdated Show resolved Hide resolved
crates/common/src/table.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! @dicej

This is one of these dense PRs that have tons of information and impl details. I enjoyed the async wasi-http-rust examlple a lot!

Made some clarifications to the wasi-http specific questions and one comment to add non-rust examples. I know it's might be difficult to do for other languages like TinyGo, but I can help if needed!

@@ -0,0 +1,290 @@
wit_bindgen::generate!("proxy" in "../../wit/preview2");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice async example for wasi-http!!

I wonder if this could be done in TinyGo with wit-bindgen-go bindings? I can help experimenting that if you don't have time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can definitely add async/await support to JS and Python. I'm not sure how it would work with TinyGo, though, since I have limited experience with Go. My understanding is that the most natural way to do concurrent I/O in Go is via goroutines, but I don't know how that's supported in TinyGo when targeting WASI or how we would hook into it with a poll_oneoff-based solution. Do you know anything about that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't. I'd like to find it out myself.


pub struct WasiCloudComponent;

impl HostComponent for WasiCloudComponent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is more like a WasiHttpComponent rather than a WasiCloudComponent. Is the intention that many more wasi-cloud-core worlds & interfaces can be add to this crate incrementally?

Suppose I want to support wasi-keyvalue, should I add wit::Keyvalue::add_to_linker... in this component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was thinking we'd add all the wasi-cloud-core state to this component (including wasi-keyvalue), partly because I expect they'll need to share the same poll and streams tables. I'm open to other ways of organizing this, though.

Copy link
Collaborator

@lann lann Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want to expose some of these common preview2 types from spin-core. I'm expecting to see them in wasmtime-wasi 10 anyway.

status: types::StatusCode,
headers: types::Headers,
) -> Result<types::OutgoingResponse> {
// TODO: What is supposed to happen if you create a new outgoing response with some headers and then edit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, I think the answer to the question "Should the response reflect those changes?" is yes, because the headers should be a resource type to fields that can be modified.

I don't think outgoing_response_write is changing the answer to that question becuase it is used to write content and trailers to the response.

Btw, these are execellent questions to be raised as issues to wasi-http and they should definitely get answered by Luke.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an issue requesting clarification: WebAssembly/wasi-http#38

this: types::OutgoingStream,
trailers: Option<types::Trailers>,
) -> Result<()> {
// TODO: We should change the WIT file so this can return an I/O error instead of trapping
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. It should return result<u64, stream-error> for the final result of outgoing-stream, right? This could be done in the wasi-http wits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an issue here: WebAssembly/wasi-http#37

This adds experimental support for the [wasi-http](https://github.com/WebAssembly/wasi-http) proposal. Note that the WIT files have been copied from that repo and modified to avoid name conflicts. Eventually, we'll want to use `wit-deps` to manage them along with all the other upstream WIT files.

The good:
 - Asynchronous, concurrent streaming supported for both inbound and outbound requests and responses
 - Includes an example that uses a custom async executor which could evolve into an async SDK API

The bad:
 - Outbound request/response trailers not yet supported since `reqwest` does not yet support them
 - Some less-used stream operations not yet implemented (e.g. `write_zeros`), but should be easy to add

The ugly:
 - Had to rename a bunch of interfaces (e.g. poll -> poll2) to avoid conflicts with our snapshot of `preview2-protyping`
   - This should be resolved once we update `wasmtime`, `wit-bindgen`, and `wasm-tools` to incorporate WebAssembly/component-model#193
 - We've commented-out the clock, random, and stdio parts of the world in favor of ambient WASI Preview 1 support, again due to the aforementioned conflicts
 - Implementing `wasi-cloud` as a host component and passing a handle to it to the wasi-http trigger is awkward
 - The host component currently traps on certain runtime errors because the WIT interface is not fallible but probably should be
 - The WIT spec is not clear on what should happen in certain corner cases (e.g. if you mutate headers after you've passed them to a constructor, or if you "finish" a stream more than once)

Long term, we'd like to contribute the Spin-agnostic parts of this implementation upstream to the Bytecode Alliance.  For now, we'll let it incubate here in Spin.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Add example + changes to get it working

Signed-off-by: Brian H <brian.hardock@fermyon.com>

first pass at inbound request implementation (untested)

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

remove accidentially-committed proxy.rs file

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

flesh out wasi-http example

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

expand `wasi-http` example to optionally use poll API

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

first draft of oubound request support (untested)

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

basic outbound request test works

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

add `wasi-http-rust-async` example

This uses a custom `async` executor to manage concurrency, which could form the
basis for an idiomatic SDK API.  It also demonstrates concurrent outbound
request handling.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

break `wasi-http-rust-async` example into smaller functions

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update `wasi-http-rust-async` example metadata

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

split `wasi-cloud` crate into separate modules

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update example descriptions

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

enable stream feature of hyper to fix CI

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

remove obsolete `cooked-waker` dependency

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

streamline `wasi-http-rust-async` example

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

add `test_wasi_http_*` tests

This updates the `wasi-http-rust-async` example to make it reusable as a test.
While I was doing that, I made a few tweaks to CI and the build files:

- Lint all the examples as part of the CI lint job so they at least build
  - ...and fix the issues Clippy flagged
  - Eventually, we'll want to also test the examples where feasible
- Relax the `cargo:rerun-if-changed` directives in build.rs to avoid spurious, expensive rebuilds
- Set PATH when running the KV test to ensure that the built `spin` is run, not the installed one

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

send request body in chunks in `test_wasi_http_echo`

This helps guarantee that the server streams the request and response rather
than buffer it.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

use dedicated error types in table.rs

This helps avoid duplicating error messages all over the `wasi-http`
implementation.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update `HttpExecutorType` doc comment

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update `HttpExecutor` doc comment

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Revert "use dedicated error types in table.rs"

This reverts commit 56e3460.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

update to latest `wasi-http` WIT files

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej
Copy link
Contributor Author

dicej commented Aug 9, 2023

Quick update on this: we'll probably never merge this PR since @pchickey and @elliottt are porting it over to the latest wasmtime-wasi streams implementation, and that port will live in Wasmtime, not here.

However, I went ahead and rebased this anyway since @Mossaka and I are preparing a presentation that will include a demo of wasi-http, wasi-key-value, and wasi-messaging, and I'm not sure if the Wasmtime wasi-http implementation will be stable by then.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej
Copy link
Contributor Author

dicej commented Aug 21, 2023

Closing this per my comment above.

@dicej dicej closed this Aug 21, 2023
dicej added a commit to dicej/spin that referenced this pull request Oct 3, 2023
These are some changes I made as part of
fermyon#1553 despite not being specifically related
to the goal of that PR.  Now I'm splitting them out into their own PR.

- Set `PATH` where appropriate to ensure the expected version of Spin is used to
  test (i.e. the one built from the current clone of the repository) rather than
  whatever happens to be installed on the user's machine.

- Lint all examples as part of CI
  - and fix the lint warnings so we have a clean baseline

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej added a commit to dicej/spin that referenced this pull request Oct 3, 2023
These are some changes I made as part of
fermyon#1553 despite not being specifically related
to the goal of that PR.  Now I'm splitting them out into their own PR.

- Set `PATH` where appropriate to ensure the expected version of Spin is used to
  test (i.e. the one built from the current clone of the repository) rather than
  whatever happens to be installed on the user's machine.

- Lint all examples as part of CI
  - and fix the lint warnings so we have a clean baseline

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
dicej added a commit to dicej/spin that referenced this pull request Oct 3, 2023
These are some changes I made as part of
fermyon#1553 despite not being specifically related
to the goal of that PR.  Now I'm splitting them out into their own PR.

- Set `PATH` where appropriate to ensure the expected version of Spin is used to
  test (i.e. the one built from the current clone of the repository) rather than
  whatever happens to be installed on the user's machine.

- Lint all examples as part of CI
  - and fix the lint warnings so we have a clean baseline

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej mentioned this pull request Oct 3, 2023
dicej added a commit that referenced this pull request Oct 3, 2023
These are some changes I made as part of
#1553 despite not being specifically related
to the goal of that PR.  Now I'm splitting them out into their own PR.

- Set `PATH` where appropriate to ensure the expected version of Spin is used to
  test (i.e. the one built from the current clone of the repository) rather than
  whatever happens to be installed on the user's machine.

- Lint all examples as part of CI
  - and fix the lint warnings so we have a clean baseline

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants