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: align wasi-http with component linker #6195

Merged

Conversation

eduardomourar
Copy link
Contributor

@eduardomourar eduardomourar commented Apr 11, 2023

In the context of wasi-http, this PR will use the shared functionality from wasmtime-wasi will prepare the ground for the migration from the preview2-prototyping repo to wasmtime. A table is being defined to simulate the future resource type. Additionally, it will simplify using Component Linker with the wasi-http module.

Usage example with WASI Preview2
use wasmtime::{
    component::{Component, Linker},
    Config, Engine, Store,
};
use wasmtime_wasi::preview2::{
    command::Command, Table, WasiCtx, WasiCtxBuilder, WasiView,
};
use wasmtime_wasi_http::{WasiHttpCtx, WasiHttpView};

pub struct Ctx {
   table: Table,
    wasi: WasiCtx,
    http: WasiHttpCtx,
}

impl WasiView for Ctx {
    fn table(&self) -> &Table {
        &self.table
    }
    fn table_mut(&mut self) -> &mut Table {
        &mut self.table
    }
    fn ctx(&self) -> &WasiCtx {
        &self.wasi
    }
    fn ctx_mut(&mut self) -> &mut WasiCtx {
        &mut self.wasi
    }
}

impl WasiHttpView for Ctx {
    fn http_ctx(&self) -> &WasiHttpCtx {
        &self.http
    }
    fn http_ctx_mut(&mut self) -> &mut WasiHttpCtx {
        &mut self.http
    }
}

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    let wasi = WasiCtxBuilder::new().inherit_stdio().build();
    let http = WasiHttpCtx::new();
    let mut config = Config::new();
    config.wasm_backtrace_details(wasmtime::WasmBacktraceDetails::Enable);
    config.wasm_component_model(true);
    config.async_support(true);

    let engine = Engine::new(&config)?;
    let component = Component::from_file(&engine, "component.wasm")?;
    let mut store = Store::new(&engine, Ctx { table, wasi, http });
    let mut linker = Linker::new(&engine);

    add_to_linker(&mut linker)?;
    wasmtime_wasi_http::add_to_component_linker(&mut linker)?;

    let (wasi, _instance) =
        wasi::Command::instantiate_async(&mut store, &component, &linker).await?;
    let result = wasi
        .call_main(&mut store, &[])
        .await
        .map_err(|e| anyhow::anyhow!("wasm failed with {e:?}"))?
        .map_err(|e| anyhow::anyhow!("command returned with failing exit status {e:?}"))?;

    Ok(())
}

Depends on: #6836.

@eduardomourar eduardomourar marked this pull request as ready for review April 18, 2023 15:53
@eduardomourar eduardomourar requested a review from a team as a code owner April 18, 2023 15:53
@eduardomourar eduardomourar requested review from alexcrichton and removed request for a team April 18, 2023 15:53
@eduardomourar
Copy link
Contributor Author

@pchickey and @brendandburns , could you review this PR, please? Ideally, we should merge this prior to #6091.

@@ -54,7 +63,7 @@ fn port_for_scheme(scheme: &Option<Scheme>) -> &str {
}
}

impl WasiHttp {
impl WasiHttpCtx {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to follow the pattern in the other modules, and also to show that there is a breaking change to the API

@brendandburns
Copy link
Contributor

I think it is probably worth splitting this into two PRs, one containing all of the common implementations and one with the refactors to the HTTP implementation.

Using the common code for things like streams seems reasonable.

I don't really see the point of the HTTP refactors, they seem to add a bunch of complexity without really reducing the amount of code that is required.

@eduardomourar
Copy link
Contributor Author

I don't really see the point of the HTTP refactors, they seem to add a bunch of complexity without really reducing the amount of code that is required.

This is important to integrate with preview2, but also to have less code change whenever the resources will be auto-generated by wit-bindgen. We will be a step further in that direction instead of diverging more and more from the expected pattern.

@pchickey
Copy link
Contributor

This looks to me like a reasonable path to align with some of the changes coming down the pipe from preview2-prototyping. It doesn't make those changes totally drop-in, but using the streams and table abstraction does take us part of the way there.

I agree that, without that context, it looks like added complexity without reducing the amount of code required.

So really the question is whether adding this now is appropriate, or if we try to land Brendan's other patches first. Unfortunately I am still deep in the preview 2 filesystem work over in p2-p and I haven't context switched back to HTTP to review Brendan's PRs yet. #6228 is pretty straightforward but #6091 has been sitting for longer and I haven't dug into it yet.

@alexcrichton
Copy link
Member

I'm going to list @pchickey as the reviewer for this since I think he's got this in his wheelhouse

@alexcrichton alexcrichton requested review from pchickey and removed request for alexcrichton April 18, 2023 18:27
@eduardomourar
Copy link
Contributor Author

@pchickey , I can understand if we want to first merge the other PRs. After they are, I will integrate the changes back here then.

@brendandburns
Copy link
Contributor

Just for my own understanding, I don't think that anything in http_impl.rs changes with the move to preview 2 does it? That struct already implements the Host trait that is defined by the wasi-http with bindings. Why does switching to Table make it more compatible? I think I'm missing something.

@eduardomourar
Copy link
Contributor Author

eduardomourar commented Apr 19, 2023

I would expect the http_impl.rs to be modified, yes. If you look into this example here and other parts of the host implementation, you will see that the table abstraction is being assumed to be available and it is the default place to store the id for each handle and associated resource instance. By doing the same for the HTTP module in this PR, we would only hold specific data in its context (right now there is none) and rely on the common context instead. This simplification is not that evident here because I copied the shared code to avoid creating a dependency to the preview2-prototyping repo.

@pchickey
Copy link
Contributor

pchickey commented Apr 19, 2023

Another wrinkle is that we'll be spreading this implementation across multiple crates. wasi-common will define the Table, a trait WasiInputStream , and then hook up all the input-stream methods in the io.streams wit to lookup in the table and downcast it to a Box<dyn WasiInputStream>, and then invoke the method.

wasmtime-wasi-http's role will be to define struct HttpBodyInputStream( {whatever the right tokio/hyper primitives here} ), impl wasi_common::WasiInputStream for HttpBodyInputStream, and then creation of a new stream will be to cast it to a Box and stick it into a table, which is shared with the wasi_common::WasiCtx.

We may end up needing to pull some additional tricks to get wasi-common and wasmtime-wasi-http to each access the table via &mut Table instead of using an Arc<Mutex<Table>> to handle sharing, but that is just an optimization we can do once the basics work.

@brendandburns
Copy link
Contributor

It seems like much of the refactor is unrelated to streams, I get that consistent stream handling makes sense, but doesn't seem like using a Table to store the request/response objects is inherently better than having a struct with multiple hashtables inside of it. Indeed it seems to complicate the implementation instead.

Moreover, given that there are multiple other wasi-modules in wasmtime that are even further from being component-linker compatible, I'd rather we focused energy on getting the HTTP implementation correct in terms of user-facing surface area and wait for preview2 to arrive in wasmtime before we worry about these kind of changes.

@eduardomourar
Copy link
Contributor Author

eduardomourar commented Apr 20, 2023

The problem is that currently we cannot use a Wasm component (+ preview2) in different runtimes (in my case wasmtime, Node.js and browser). I guess other people/companies are facing the same issue.

The way I see, we have two options:

  • we go forward with this PR (after the pending PRs are merged, obviously) by bringing preview2 partially here;
  • or we move all this logic to the preview2-prototyping repo;

I actually preferred the second option, which was the one decided originally. Although, you can take this PR as a compromise where we will temporarily maintain those in two places in order to bring the learnings upstream and they will eventually converge.

@eduardomourar eduardomourar force-pushed the feat/wasi-http-align-component branch from 340056a to 7ddf592 Compare July 31, 2023 10:08
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Aug 4, 2023
@eduardomourar eduardomourar requested a review from a team as a code owner August 5, 2023 15:14
@eduardomourar eduardomourar requested review from elliottt and removed request for a team August 5, 2023 15:14
@elliottt elliottt added this pull request to the merge queue Aug 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023
@elliottt elliottt force-pushed the feat/wasi-http-align-component branch from fddc0a1 to c374996 Compare August 17, 2023 22:53
@elliottt elliottt force-pushed the feat/wasi-http-align-component branch from c374996 to 33400f6 Compare August 17, 2023 23:09
@elliottt elliottt force-pushed the feat/wasi-http-align-component branch from 80773ae to 2a3e90f Compare August 18, 2023 00:45
@elliottt elliottt added this pull request to the merge queue Aug 18, 2023
@elliottt elliottt removed this pull request from the merge queue due to a manual request Aug 18, 2023
@elliottt elliottt added this pull request to the merge queue Aug 18, 2023
Merged via the queue into bytecodealliance:main with commit e250334 Aug 18, 2023
31 checks passed
@eduardomourar eduardomourar deleted the feat/wasi-http-align-component branch August 18, 2023 09:03
eduardomourar added a commit to eduardomourar/wasmtime that referenced this pull request Aug 18, 2023
* feat: align wasi-http with component linker

* feat(wasi-http): allow bidirectional stream

* feat(wasi-http): clean up children when dropping resource

* chore: update based on feedback

* chore: replace wasi http context references

* chore: fix logical issue with outgoing body stream

* chore: use thread-safe reference-counting pointer

* chore: cleanup resources using table

* fix(wasi-preview1-component-adapter): sync command extended wit

* fix(wasi-preview1-component-adapter): sync command extended wit

* fix(wasmtime-wasi): sync wit for http types

* chore: refactor using wasmtime-wasi crate

fix(wasi-http): misconfiguration in wasmtime linkers

chore: keep streams details

chore: fix wasi http tests

* chore: use pollable from wasmtime-wasi

* chore: update wasi http linker for module

* chore: update test programs for wasi http

* fix(wasi-http): ensure proper errors are surfaced

* chore: split wasi http tests into individual files

* chore: ensure protocol error is mapped correctly

* chore: disable temporarily wasi http in wasmtime cli

* chore: comment out wasi http in wasmtime cli

* chore(ci): ensure wit definitions in sync

* feat(wasi-http): generate async host binding

* chore: make wasi http tests async

* chore: update ci workflow based on suggestion

Co-authored-by: Pat Hickey <pat@moreproductive.org>

* feat(wasmtime-wasi): update logging world to latest

* feat(wasmtime): update proxy world to latest

* feat(wasmtime-wasi): add back command extended world

* fix(wasi-http): sync wit definitions

* chore: update tests with latest wit definitions

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* Update src/commands/run.rs

* chore: fix formatting

* Ignore flaky test

* chore: fix compilation error for riscv64 arch

* Avoid `cp -T` on macos

Adding prtest:full to ensure that we've seen a successful build before
queuing.

* Don't build the wasi-http test programs for the native target

* Debug the wit consistency check

* Update streams.wit in wasi-http

* Mark the component outbound_request_post test flaky

* Disable flaky wasi-http-tests on windows only

* Use diff instead of rm/cp/git diff

* Disable more tests on windows

---------

Co-authored-by: Eduardo Rodrigues <eduardomourar@users.noreply.github.com>
Co-authored-by: Pat Hickey <pat@moreproductive.org>
Co-authored-by: Trevor Elliott <telliott@fastly.com>
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Sep 12, 2023
This was removed in bytecodealliance#6195 but re-added in bytecodealliance#6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Sep 12, 2023
This was removed in bytecodealliance#6195 but re-added in bytecodealliance#6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.
pchickey pushed a commit that referenced this pull request Sep 12, 2023
This was removed in #6195 but re-added in #6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2023
This was removed in #6195 but re-added in #6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.
pchickey pushed a commit that referenced this pull request Sep 13, 2023
…13.0 (#7009)

* WASI preview 2 output-streams: new backpressure and flushing design (#6877)

* Stream backpressure v2

Co-authored-by: Pat Hickey <phickey@fastly.com>
Co-authored-by: Trevor Elliott <telliott@fastly.com>
Co-authored-by: Dan Gohman <dev@sunfishcode.online>

Stop testing pseudocode

Restructure when notifications are sent, and make sure to flush the writer

Fix the wasi-http module versions of flush and blocking_flush

Use blocking_write_and_flush for blocking writes in the adapters

Fix a warning in wasi-http

Remove an unused DropPollable

add comment explaining try_write for tcpstream

refactor: separate struct for representing TcpReadStream

by factoring into HostTcpSocket a little bit

tcp read stream: handle stream closing

tcp tests: use blocking_read where its expecting to wait for input

move common test body into wasi-sockets-tests/src/lib.rs

ensure parent socket outlives pollable

input and output streams can be children now

tcp's streams are the sockets children

tcp.wit: document child relationships

tcp tests: fix to drop socket after its child streams

review feedback: propogate worker task panic

style

error source fix

tcp: use preview2::spawn, and propogate worker panics

join handle await always propogates panic

background task handles ewouldblock as well

document choice of constant

* sync wit notes into wasi-http

* improve wit docs for output-stream

* doc: document `HostOutputStream` (#6980)

* doc: document `HostOutputStream`

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Co-authored-by: Pat Hickey <pat@moreproductive.org>

* fix(wasi): fail when `MemoryOutputStream` buffer is full

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

---------

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Co-authored-by: Pat Hickey <pat@moreproductive.org>

* rustfmt

prtest:full

* windows and doc fixes

* cli test wasi-http: use blocking-write-and-flush

* Disable some tests, and adjust timeouts when running under qemu

* Try to reproduce the riscv64 failures

* Update riscv to LLVM 17 with beta rust

* Revert "Try to reproduce the riscv64 failures"

This reverts commit 8ac6781.

* Pin the beta version for riscv64

* Fix a warning on nightly

---------

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Co-authored-by: Roman Volosatovs <rvolosatovs@users.noreply.github.com>
Co-authored-by: Trevor Elliott <telliott@fastly.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>

* Un-ignore now-passing test

With the merging of #6877 prints to stdout with preview2 should now work
without requiring extra sleeps or such.

* Remove submodule re-added by accident

This was removed in #6195 but re-added in #6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.

* add to release notes

---------

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Co-authored-by: Roman Volosatovs <rvolosatovs@users.noreply.github.com>
Co-authored-by: Trevor Elliott <telliott@fastly.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 13, 2023
This was removed in bytecodealliance#6195 but re-added in bytecodealliance#6877, I believe by accident,
so this re-deletes it. I've also edited `.gitmodules` a bit while I was
here to remove it and additionally keep other entries up-to-date with
matching paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants