brendandburns edited PR #5929 from wasi_http
to main
:
This is a major WIP, no need to review currently, mostly here so I can visualize the diff and checkpoint it.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- Body for requests is not supported
- Request options (e.g. timeout values) are not supported.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft, working, but with limitations.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- Body for requests is not supported
- Request options (e.g. timeout values) are not supported.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft, working, but with limitations.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- Body for requests is not supported
- Request options (e.g. timeout values) are not supported.
- There are panics all over the place if you pass in bad pointers or unexpected values.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft, working, but with limitations.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- ~Body for requests is not supported~
- Request options (e.g. timeout values) are not supported.
- There are panics all over the place if you pass in bad pointers or unexpected values.
brendandburns updated PR #5929 from wasi_http
to main
.
pchickey submitted PR review.
pchickey submitted PR review.
pchickey created PR review comment:
This shouldn't be a macro - lets write it as a regular function?
pchickey created PR review comment:
alignment is set to 0 here, which will trap in some allocators
pchickey created PR review comment:
this method isn't expressible in wit, so we don't have an off-ramp into the component runtime for it. I do not think we should leave it in, since the idea is we will replace all of the functionality with the component runtime once the other pieces are ready.
pchickey created PR review comment:
We have a project policy against use of
unsafe
- additionally, this will be incorrect on big-endian (s390x). Can you instead useu32::to_le_bytes()
to write these values into a byte array?
pchickey created PR review comment:
We use
authors.workspace = true
to pickup the "The Wasmtime Project Developers" author
pchickey created PR review comment:
I'm not sure if we'll want to use this name for the host implementation, or reserve it for guest side bindings, but either way I grabbed the name on crates.io and we can land it with this name for now
pchickey created PR review comment:
edition.workspace = true ```` ~~~
pchickey created PR review comment:
thiserror = { workspace = true }
pchickey created PR review comment:
anyhow = { workspace = true }
pchickey created PR review comment:
Project uses
Apache-2.0 WITH LLVM-exception
pchickey created PR review comment:
I'm a bit surprised all these functions landed in the
types
module name, that may be something we have to look into...
brendandburns submitted PR review.
brendandburns created PR review comment:
The compiler doesn't like this function b/c of Rust borrow semantics when I write it as a function:
error[E0382]: borrow of moved value: `caller` --> crates/wasi-http/src/component_impl.rs:343:37 | 330 | move |mut caller: Caller<'_, T>, stream: u32, len: u64, ptr: u32| { | ---------- move occurs because `caller` has type `Caller<'_, T>`, which does not implement the `Copy` trait ... 339 | let out_ptr = allocate_guest_pointer(caller, body_len); | ------ value moved here ... 343 | let memory = memory_get(&mut caller).unwrap(); | ^^^^^^^^^^^ value borrowed here after move error[E0382]: use of moved value: `caller` --> crates/wasi-http/src/component_impl.rs:390:55 | 377 | move |mut caller: Caller<'_, T>, fields: u32, out_ptr: u32| { | ---------- move occurs because `caller` has type `Caller<'_, T>`, which does not implement the `Copy` trait ... 382 | let tuple_ptr = allocate_guest_pointer(caller, (16 * header_len).try_into().unwrap()); | ------ value moved here ... 390 | let name_ptr = allocate_guest_pointer(caller, name_len); | ^^^^^^ value used here after move error[E0382]: use of moved value: `caller` --> crates/wasi-http/src/component_impl.rs:391:56 | 377 | move |mut caller: Caller<'_, T>, fields: u32, out_ptr: u32| { | ---------- move occurs because `caller` has type `Caller<'_, T>`, which does not implement the `Copy` trait ... 390 | let name_ptr = allocate_guest_pointer(caller, name_len); | ------ value moved here 391 | let value_ptr = allocate_guest_pointer(caller, value_len); | ^^^^^^ value used here after move error[E0382]: borrow of moved value: `caller` --> crates/wasi-http/src/component_impl.rs:393:41 | 377 | move |mut caller: Caller<'_, T>, fields: u32, out_ptr: u32| { | ---------- move occurs because `caller` has type `Caller<'_, T>`, which does not implement the `Copy` trait ... 391 | let value_ptr = allocate_guest_pointer(caller, value_len); | ------ value moved here 392 | 393 | let memory = memory_get(&mut caller).unwrap(); | ^^^^^^^^^^^ value borrowed here after move For more information about this error, try `rustc --explain E0382`. error: could not compile `wasi-http` due to 4 previous errors
Admittedly I'm a Rust newb, so if there is a way to resolve this, please let me know. I tried a few things but I couldn't see a way to do it.
brendandburns edited PR review comment.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns submitted PR review.
brendandburns created PR review comment:
switched to 4
brendandburns submitted PR review.
brendandburns created PR review comment:
removed, this was stale old code.
brendandburns submitted PR review.
brendandburns created PR review comment:
Done (here and below)
brendandburns submitted PR review.
brendandburns created PR review comment:
done.
brendandburns created PR review comment:
done.
brendandburns submitted PR review.
brendandburns submitted PR review.
brendandburns created PR review comment:
done.
brendandburns submitted PR review.
brendandburns created PR review comment:
done.
brendandburns submitted PR review.
brendandburns created PR review comment:
acknowledged.
brendandburns submitted PR review.
brendandburns created PR review comment:
done.
brendandburns submitted PR review.
brendandburns created PR review comment:
yeah, surprised me to, seems like some sort of aliasing in
wit-bindgen
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns created PR review comment:
I figured out how to fix this, so this is now fixed in the PR.
brendandburns submitted PR review.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft, working, but with limitations.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- ~Body for requests is not supported~
- Request options (e.g. timeout values) are not supported.
- ~There are panics all over the place if you pass in bad pointers or unexpected values.~
brendandburns updated PR #5929 from wasi_http
to main
.
Mossaka submitted PR review.
Mossaka submitted PR review.
Mossaka created PR review comment:
_ => return bail!("unknown method!"),
Mossaka created PR review comment:
If you set
async: true
then you can usereqwest::Client
instead ofreqwest::blocking::Client
.
pchickey submitted PR review.
pchickey created PR review comment:
Agreed - really, the only time we want to use the sync traits is to support hosts that arent running tokio. Since the reqwest blocking client is actually using tokio under the hood, using the sync trait means this library can't compose with the rest of the wasi libraries when run on tokio, but it also can't work for hosts that don't want to incur tokio, which is the worst of both worlds.
And once we make that change, I'd much prefer to drop down to just plain old hyper, because I don't think there's a ton of value in the reqwest async interface over the hyper primitives for this use case, and reqwest is responsible for at least half of the dependencies that need to be
cargo vet
ed in order to land this.
pchickey edited PR review comment.
pchickey edited PR review comment.
brendandburns submitted PR review.
brendandburns created PR review comment:
I looked at hyper. It makes you do things like explicitly open a TCP connection before talking HTTP, it's ergonomics as a library are not great at first glance anyway.
I'm happy to do this as a later optimization, but I don't think that it impacts this PR, so I'd prefer to merge and iterate.
pchickey submitted PR review.
pchickey created PR review comment:
In order to land the PR I have to audit all unvetted dependencies for supply chain safety. These audits are very time consuming - I have to review the source of the entire crate. We can only accept these audits if they are performed by core team members. I can't make any kind of exception for this PR, or breeze through things because reqwest will be swapped out soon, because our audits are imported to be part of the supply chain safety of other projects.
Right now this PR introduces 49 unvetted dependencies into the project https://github.com/bytecodealliance/wasmtime/actions/runs/4430843524/jobs/7773090032?pr=5929. That represents at least a full day of very tedious work for me. Many of those dependencies are pulled in exclusively by the reqwest library, in order to provide functionality we don't need. (Because of the use by other projects, vetting considers all dependencies for all features and targets, even though our project doesn't need them.)
Therefore, we need to iterate on this before landing the PR. Like I said, I can send a PR to your branch to help with this transformation, because my time would be better spent on making the changes that need to happen anyway vs auditing packages we don't care about.
brendandburns submitted PR review.
brendandburns created PR review comment:
The need to reduce the scope of rust vet makes total sense to me. I will look into switching this to Hyper.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns edited PR #5929 from wasi_http
to main
:
Initial draft, working, but with limitations.
Known issues:
- Outbound requests only, inbound HTTP serving is not supported
- ~Body for requests is not supported~
- ~Request options (e.g. timeout values) are not supported.~
- ~There are panics all over the place if you pass in bad pointers or unexpected values.~
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
eduardomourar submitted PR review.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
i believe this was a typo
http-body = "1.0.0-rc.2"
eduardomourar created PR review comment:
we don't need the full set of features
tokio = { version = "1", default-features = false, features = ["net", "rt-multi-thread", "time"] }
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
eduardomourar submitted PR review.
Mossaka submitted PR review.
Mossaka submitted PR review.
Mossaka created PR review comment:
It doesn't seem like the
len
parameter has been used here to specify how much bytes to read. I feel like for an initial implementation this should be okay, but if the response body is too big to fit in host's memory then we probably need to useBufRead
to read bytes into a buffer incrementally.I remember you added
read_into
function before, but I can't find it anymore.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns submitted PR review.
brendandburns created PR review comment:
read_into
was removed after feedback from @pchickey and others.I fixed this function to actually implement read correctly.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns requested pchickey for a review on PR #5929.
brendandburns updated PR #5929 from wasi_http
to main
.
brendandburns requested wasmtime-default-reviewers for a review on PR #5929.
brendandburns requested wasmtime-core-reviewers for a review on PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
pchickey submitted PR review.
pchickey submitted PR review.
pchickey created PR review comment:
let builder = WasiCtxBuilder::new().inherit_stdio().arg(bin_name)?;
pchickey created PR review comment:
let config = Config::new();
pchickey created PR review comment:
_workspace: Option<&Path>,
pchickey updated PR #5929.
pchickey updated PR #5929.
pchickey updated PR #5929.
pchickey submitted PR review.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
I know this is a little bit late, but can we change this package name to
wasmtime-wasi-http
, please?
eduardomourar deleted PR review comment.
brendandburns updated PR #5929.
pchickey submitted PR review.
pchickey submitted PR review.
pchickey created PR review comment:
#![allow(dead_code, unused_imports)]
pchickey updated PR #5929.
pchickey has enabled auto merge for PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
rvolosatovs submitted PR review.
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Shouldn't need SSL anymore, right?
brendandburns updated PR #5929.
brendandburns submitted PR review.
brendandburns created PR review comment:
done.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
eduardomourar created PR review comment:
we should actually be returning the future incoming response id here and not the response id
eduardomourar submitted PR review.
brendandburns submitted PR review.
brendandburns created PR review comment:
Future's aren't implemented (yet) in this PR so the response_id is the same as the "future" id. If you look at the future-incoming-response-get in component_impl.rs, you'll see that it just returns the passed in future as the response id.
Eventually we will implement futures and this will change, but not in this PR.
pchickey submitted PR review.
eduardomourar submitted PR review.
eduardomourar created PR review comment:
after this is merged, i will submit a pr that will at least prevent the host from crashing if the emulated future is assumed by the guest like here: https://github.com/bytecodealliance/jco/blob/2b3c9cd7839b3728f74af2cbf6722cf09287f956/packages/preview2-shim/lib/http/wasi-http.js#L263-L296
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
brendandburns updated PR #5929.
pchickey submitted PR review.
pchickey created PR review comment:
# The `ring` crate, used to implement TLS, does not build on riscv64 or s390x [target.'cfg(not(any(target_arch = "riscv64", target_arch = "s390x")))'.dependencies]
pchickey updated PR #5929.
pchickey submitted PR review.
pchickey has enabled auto merge for PR #5929.
pchickey merged PR #5929.
Last updated: Nov 22 2024 at 16:03 UTC