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

Add aws-smithy-wasm crate with WASI http client #3409

Merged
merged 90 commits into from
Feb 28, 2024
Merged

Add aws-smithy-wasm crate with WASI http client #3409

merged 90 commits into from
Feb 28, 2024

Conversation

landonxjames
Copy link
Contributor

@landonxjames landonxjames commented Feb 14, 2024

Motivation and Context

This change adds a new crate, aws-smithy-wasm, that exports a SDK compatible WASI http client. This is a continuation of the work in #2520 using the now stabilized WASI 0.2.0 interfaces from the wasi crate. This supports, but does not finalize the work for #2087

Description

Add a new crate, aws-smithy-wasm which exports a function wasi_http_client that will provide the user with a WASI compatible http client. This client is implemented by using the wasi::http::outgoing_handler ref along with some utility implementations of TryFrom to transform back and worth between the types from the http crate and the wasi::http types. It also exports a unit struct WasmSleep that impls the AsyncSleep trait needed by the SDK.

Testing

This is tested via an integration test in aws/sdk/integration-tests/webassembly that uses the wasi http-client to vuild a config and an operation (that is not sent). It is further tested in a new canary (wasm_canary) that calls the S3 list_objects_v2 API.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Added a Builder to allow for future configuration to be baked in in a
backwards compatible manner. Also updated some of the error handling.

Updated the associated tests to use the new Builder and to not send a
network call. Sending the actual network call will be moved to the
canary in another commit.

Undoing some autoformatting in the Dockerfile

Lint fixes
It compiles and the wasm runs, although unsuccessfully due to some
missing WASI imports. Still trying to figure that out.
Unfortunately having trouble making the canary run `async` because there
is a tokio error.
@landonxjames
Copy link
Contributor Author

Ok pending this final CI run this should be all good to go. As requested I added a new canary wasm_canary which uses the wasmtime runtime to run a wasm file. The code for the wasm file lives in tools/ci-cdk/canary-wasm mostly because a separate crate was much easier to compile. The built wasm binary is bundled into the .zip thats deployed to the Lambda.

Currently the canary only tests a simple list_objects_v2 with no_credentials set on the config. Note that I had to bump up the timeouts for the canary pretty substantially to get all of this to work. It was taking over a minute just to compile and instantiate the wasm binary. This could probably be improved with a beefier lambda instance, but I thought I would leave that choice up to the SDK team.

Note: There is one CI task that continues to fail, the check-aws-sdk-cargo-deny task. It seems like something about the Cargo.toml in aws-amithy-wasm (I suspect the target statement for the dependencies) is causing the versions of the deps not to be resolved during the :aws:sdk:assemble step.

rust-runtime/aws-smithy-wasm/Cargo.toml Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-wasm/src/wasi.rs Show resolved Hide resolved
rust-runtime/aws-smithy-wasm/src/wasi.rs Show resolved Hide resolved
Comment on lines +77 to +80
let converted_req = http_req.map(|body| match body.bytes() {
Some(value) => Bytes::copy_from_slice(value),
None => Bytes::new(),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are streaming request bodies possible in WASI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not yet possible, but they will be when WASI preview 3 launches. Streaming bodies are waiting on native async support in wasm components. There is some detail around streaming bodies in the proposal for async/preview 3

Comment on lines +155 to +157
let read_timeout = value
.read_timeout()
.map(|dur| u64::try_from(dur.as_nanos()).unwrap_or(u64::MAX));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a difference between not calling the set_*_timeout methods and setting it to u64::MAX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they aren't called the default appears to be 600,000 milliseconds (set here). But this is set in wasmtime and not by the WASI bindings, so it seems to be host dependent. To be consistent with that I can change the u64::MAX to match their 600 seconds default.

Copy link
Contributor Author

@landonxjames landonxjames Feb 26, 2024

Choose a reason for hiding this comment

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

Breaking this down a little more:

The behavior of set_*_timeout:

  • set_*_timeout is never called - then the default timeout (for all of them) is set to 600 seconds (code). Note that we don't encounter this in our code since the timeouts are always set based on the HttpConnectorSettings values.
  • set_*_timeout is called with None - the timeout is set to 0 (code)
  • set_*_timeout is called with Some(u64) - timeout is set to the u64 value

The behavior of WasiRequestOptions::from(HttpConnectorSettings):

  • The HttpConnectorSettings contains a timeout value (u128) less than u64::MAX - Then the corresponding timeout value on outgoing_handler::RequestOptions is set to that value (cast to a u64)
  • The HttpConnectorSettings contains a timeout value (u128) greater than u64::MAX - Then the value is clamped to u64::MAX and set to that
  • The HttpConnectorSettings contains a None for the timeout - set_*_timeout is called with None and thus the timeout is set to 0 (see above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments explaining the clamping in c883aad

rust-runtime/aws-smithy-wasm/src/wasm.rs Outdated Show resolved Hide resolved
tools/ci-cdk/canary-runner/src/build_bundle.rs Outdated Show resolved Hide resolved
tools/ci-cdk/canary-runner/src/build_bundle.rs Outdated Show resolved Hide resolved
tools/ci-cdk/canary-runner/src/build_bundle.rs Outdated Show resolved Hide resolved
landonxjames and others added 3 commits February 26, 2024 11:58
Remove WasmSleep in favor of existing TokioSleep

Update the targeting in wasm crates dependencies

Updating comments around setting timeouts
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Code changes look good to me! Just need to get CI passing at this point.

Moving wasmtime-cli off nightly since MSRV bump

Removing all dependency targeting to appease tests

Removing the target on the deps in aws-smithy-wasm's Cargo.toml caused
the cargo udeps tests to fail indicating those were unused dependencies.
This was caused by the target clause on pub mod wasi in the crate's
lib.rs, so that also had to be removed.
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge February 27, 2024 22:36
@jdisanti jdisanti added this pull request to the merge queue Feb 27, 2024
Merged via the queue into smithy-lang:main with commit d95cc86 Feb 28, 2024
38 of 39 checks passed
@landonxjames landonxjames deleted the landonxjames/wasi-2-http branch February 28, 2024 17:47
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