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

feat(wasmtime-cli): restore support for wasi http module #6878

Merged

Conversation

eduardomourar
Copy link
Contributor

@eduardomourar eduardomourar commented Aug 22, 2023

Within the wasmtime CLI, the current default behavior is to only inject the synchronous functions to linkers. This will add a flag called --async that will inject the asynchronous one instead.

This is required in order to enable back the wasi-http module because it has been implemented only with async functions. Additionally, it will help the async support code path to be tested as async should be enabled by default in the future.

This will enable back the wasi-http module within the wasmtime CLI. We will make the bindgen synchronous through the tokio executor available in wasmtime.

@eduardomourar eduardomourar requested review from a team as code owners August 22, 2023 11:17
@eduardomourar eduardomourar requested review from jameysharp and removed request for a team August 22, 2023 11:17
@eduardomourar eduardomourar force-pushed the feat/wasmtime-cli-async-support branch from fd6cecb to 9148521 Compare August 22, 2023 11:27
@eduardomourar eduardomourar changed the title feat(wasmtime-cli): add async support flag feat(wasmtime-cli): add flag for async support Aug 22, 2023
@eduardomourar eduardomourar force-pushed the feat/wasmtime-cli-async-support branch from 9148521 to b743ff2 Compare August 22, 2023 11:31
@alexcrichton
Copy link
Member

Thanks! Could you expand a bit more on the rationale for this? Why would users of the CLI want to control async-vs-not? Or is this instead intended for testing the async implementations on the CLI?

@eduardomourar
Copy link
Contributor Author

Thanks! Could you expand a bit more on the rationale for this? Why would users of the CLI want to control async-vs-not? Or is this instead intended for testing the async implementations on the CLI?

Just added more details to the PR body itself.

@pchickey
Copy link
Contributor

This isn't the correct approach - instead, wasmtime-wasi::preview2::in_tokio should be made pub, and and the wasmtime-wasi-http crate should use that to implement a sync bindgen trait by wrapping the async bindgen trait.

futures::executor isnt the right executor to use with wasmtime-wasi, it will fall over if not used with tokio.

@eduardomourar eduardomourar marked this pull request as draft August 23, 2023 00:37
@eduardomourar eduardomourar changed the title feat(wasmtime-cli): add flag for async support feat(wasmtime-cli): add back support for wasi http module Aug 23, 2023
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Aug 25, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasi", "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@eduardomourar eduardomourar changed the title feat(wasmtime-cli): add back support for wasi http module feat(wasmtime-cli): restore support for wasi http module Aug 25, 2023
@eduardomourar eduardomourar force-pushed the feat/wasmtime-cli-async-support branch from 08c09dc to 9401c72 Compare August 26, 2023 00:21
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

With a few changes we can accept this PR, thanks!

Just as a heads up, I know you put a lot of labor into keeping the core-wasm wasmtime::Linker version of this code working, including the sync side of the bindings. We'll keep those working for the wasmtime 13 release, but after that we are going to swap the implementation over to use wasmtime-wasi::preview2 streams and pollables, as well as component resource types. When we make that switch, we will deprecate the core-wasm interfaces, since we don't want to maintain both.

i32.const 0 ;; base pointer
i32.const 0 ;; length
))
(local.set $request_id (call $__wasi_http_types_new_outgoing_request
Copy link
Contributor

Choose a reason for hiding this comment

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

this is impressive! nice job

}
#[cfg(feature = "component-model")]
CliLinker::Component(linker) => {
wasmtime_wasi_http::proxy::sync::add_to_linker(linker)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using wasi-http with the component::Linker will fail because it conflicts with the stream methods in the wasmtime-wasi::preview2 implementation. So, we should instead bail in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is actually not case because we are taking the implementation from the wasmtime-wasi crate when building the bindings:

"wasi:io/streams": preview2::bindings::sync_io::io::streams,
"wasi:poll/poll": preview2::bindings::sync_io::poll::poll,

we should be able to migrate the cli test to a component when the module code is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, but without actually using HostInputStream and HostOutputStream, it will still crash at runtime.

crates/wasmtime/Cargo.toml Outdated Show resolved Hide resolved
crates/wasi-http/src/http_impl.rs Outdated Show resolved Hide resolved
@pchickey pchickey enabled auto-merge September 1, 2023 18:50
@pchickey pchickey added this pull request to the merge queue Sep 1, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 1, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Sep 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 5, 2023
@elliottt elliottt added this pull request to the merge queue Sep 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 5, 2023
@pchickey
Copy link
Contributor

pchickey commented Sep 5, 2023

Looks like there is some sort of cargo issue that happens when building the cli, which doesnt happen in the usual set of tests that runs before the merge queue runs the full set. Put the string prtest:full in a commit message to get the full test suite running each time you push.

@eduardomourar eduardomourar force-pushed the feat/wasmtime-cli-async-support branch from 9c3d9af to 968da10 Compare September 5, 2023 19:01
@eduardomourar eduardomourar force-pushed the feat/wasmtime-cli-async-support branch from 968da10 to e56a4dd Compare September 5, 2023 20:00
@eduardomourar
Copy link
Contributor Author

eduardomourar commented Sep 5, 2023

@pchickey, now tests are passing. I had to ignore the wasi-http tests on Windows as they were failing. Can we merge this, please? I plan on further debugging those after PR #6877 lands.

@pchickey pchickey enabled auto-merge September 5, 2023 22:24
@pchickey pchickey added this pull request to the merge queue Sep 5, 2023
Merged via the queue into bytecodealliance:main with commit 8eefa7c Sep 5, 2023
@eduardomourar eduardomourar deleted the feat/wasmtime-cli-async-support branch September 6, 2023 00:26
@eduardomourar
Copy link
Contributor Author

@alexcrichton and @pchickey, could wasi-http be enabled back in the wasmtime v13 release branch, please? Otherwise, there will be no release with module support for this.

@alexcrichton
Copy link
Member

AFAIK that was the plan yeah, so seems reasonable to me. The process for doing so would require a PR to the release-13.0.0 branch of Wasmtime. @eduardomourar are you ok doing that?

eduardomourar added a commit to eduardomourar/wasmtime that referenced this pull request Sep 6, 2023
…iance#6878)

* feat(wasmtime-cli): add async support flag

Within the wasmtime CLI, the current default behavior
is to only inject the synchronous functions to linkers.
This will add a flag called `--async` that will inject
the asynchronous one instead.

* chore: refactor wasi http crate

* feat(wasmtime-wasi): make in_tokio function public

* feat(wasi-http): define default feature called sync

* Revert "feat(wasmtime-cli): add async support flag"

This reverts commit b743ff2.

* chore: improve flaky tests for wasi http

* feat(wasi-http): expose sync api for components

* chore: add tests for sync api of wasi http components

* feat(wasmtime-cli): restore support for wasi http module

* chore: revert change to outbound http request invalid test

* chore: have extra tracing to help debugging

* feat(wasi-http): allow modules with sync functions in linker

* fix(wasi-http): missing response body in sync api

* feat: include blocking for io streams

* chore: add tests for wasi http module in cli

* chore: disable preview2 flag in wasi http test

* chore: use preview2 flag in wasi http test

* fix(wasi-http): missing stream output in sync api

* chore: fix tests for wasi http

* chore: add tracing for poll oneoff call

* chore: send exit signal on wasi http test

* chore: swap println to tracing debug

* chore: set http server timeout to 50 secs by default

* chore: add test posting large file

* chore: revert formatting in cargo toml

* chore: fix wasi-http feature and skip failing tests

prtest:full

---------

Co-authored-by: Eduardo Rodrigues <[email protected]>
eduardomourar added a commit to eduardomourar/wasmtime that referenced this pull request Sep 6, 2023
…iance#6878)

* feat(wasmtime-cli): add async support flag

Within the wasmtime CLI, the current default behavior
is to only inject the synchronous functions to linkers.
This will add a flag called `--async` that will inject
the asynchronous one instead.

* chore: refactor wasi http crate

* feat(wasmtime-wasi): make in_tokio function public

* feat(wasi-http): define default feature called sync

* Revert "feat(wasmtime-cli): add async support flag"

This reverts commit b743ff2.

* chore: improve flaky tests for wasi http

* feat(wasi-http): expose sync api for components

* chore: add tests for sync api of wasi http components

* feat(wasmtime-cli): restore support for wasi http module

* chore: revert change to outbound http request invalid test

* chore: have extra tracing to help debugging

* feat(wasi-http): allow modules with sync functions in linker

* fix(wasi-http): missing response body in sync api

* feat: include blocking for io streams

* chore: add tests for wasi http module in cli

* chore: disable preview2 flag in wasi http test

* chore: use preview2 flag in wasi http test

* fix(wasi-http): missing stream output in sync api

* chore: fix tests for wasi http

* chore: add tracing for poll oneoff call

* chore: send exit signal on wasi http test

* chore: swap println to tracing debug

* chore: set http server timeout to 50 secs by default

* chore: add test posting large file

* chore: revert formatting in cargo toml

* chore: fix wasi-http feature and skip failing tests

prtest:full

---------

Co-authored-by: Eduardo Rodrigues <[email protected]>
pchickey pushed a commit that referenced this pull request Sep 11, 2023
* feat(wasmtime-cli): add async support flag

Within the wasmtime CLI, the current default behavior
is to only inject the synchronous functions to linkers.
This will add a flag called `--async` that will inject
the asynchronous one instead.

* chore: refactor wasi http crate

* feat(wasmtime-wasi): make in_tokio function public

* feat(wasi-http): define default feature called sync

* Revert "feat(wasmtime-cli): add async support flag"

This reverts commit b743ff2.

* chore: improve flaky tests for wasi http

* feat(wasi-http): expose sync api for components

* chore: add tests for sync api of wasi http components

* feat(wasmtime-cli): restore support for wasi http module

* chore: revert change to outbound http request invalid test

* chore: have extra tracing to help debugging

* feat(wasi-http): allow modules with sync functions in linker

* fix(wasi-http): missing response body in sync api

* feat: include blocking for io streams

* chore: add tests for wasi http module in cli

* chore: disable preview2 flag in wasi http test

* chore: use preview2 flag in wasi http test

* fix(wasi-http): missing stream output in sync api

* chore: fix tests for wasi http

* chore: add tracing for poll oneoff call

* chore: send exit signal on wasi http test

* chore: swap println to tracing debug

* chore: set http server timeout to 50 secs by default

* chore: add test posting large file

* chore: revert formatting in cargo toml

* chore: fix wasi-http feature and skip failing tests

prtest:full

---------

Co-authored-by: Eduardo Rodrigues <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants