Is anyone around who can explain how the bindings
module in crates/wasi/src/preview2/mod.rs works?
I can try giving it a stab!
although as I read it now I fear I may not be of much help
oh now I'm thinking it may be to have only some methods async
I'm gonna fiddle
i could explain it, but im not really around
but i can be around :)
its convoluted, because when i wrote it we didn't have a way to say "just make some of these generated bindings async, keep the rest sync". i think alex did actually add that recently
I got some errors where I needed to use _internal_clocks::wasi::io::poll::Pollable
insteaed of _internal_io::wasi::io::poll::Pollable
.
but basically: for the poll, stream, and filesystem interfaces, we do actually need to do tokio-async things in the methods. (we shouldn't in filesystem, but we do for tokio implementation reasons, to move the blocking calls to a worker thread while the thread running the host call itself can yield)
oh
hmm.
i think this is probably something about that approach that broke when we moved to resources.
it used to be those types were "the same" because they were type Pollable = u32
in both modules.
I've tried to clean this up a bit in this PR (sorry just came back here)
but now theyre unique t ypes in each position
ah, that makes sense
I think that can be fixed with more stuff specified in with: { ... }
the idea is that with
should allow you to redirect any types you are use
ing
yeah
my above PR should also fix it since there's only one invocation of bindgen!
modulo the sync bits which may need a with: { ... }
yes, so the other thing going on in there is we need sync interfaces that dispatch to the async interfaces via preview2's private tokio runtime, in order to support users of wasmtime who are in synchronous rust
those should be able to use with
thanks for the cleanup PR alex
Should read
and write
be sync, since they're non-blocking?
i think read
still is technically async for filesystem reads
but write
can maybe be sync now?
filesystem writes got taken care of by the flush changes, but theres still a special case for reads where they need to be done with a blocking syscall because epoll cant do files
so they need to get moved to the blocking thread (spawn_blocking)
I was under the impression that even read
was non-blocking because tokio would read into a buffer on another thread
we cant do things that way for... reasons
i forget what they are, precisely, at this exact moment
but i remember alex and i scratched our heads pretty hard about this earlier this summer and we decided we cant do it
it's probably worth revisiting now yeah though
in theory I think read/write should be able to be sync methods in rust since they can't block in rust
might require some fiddling in the source since I think I just saw an .await
in there
ok. im gonna step away and let you two take care of revisiting that for now, and go back to http streams
but first ill sign off that PR above
I cherry-picked that PR into my local branch and it's Working For Me
Oh, also, the reason read
and write
need be async is that those are the file read and write methods, which are basically pread
and pwrite
, so yeah, they're doing I/O, and should be async in the bindings.
So it's all good.
I've got a follow-up commit though to remove async from check-write
, flush
, and write-zeros
technically we could make write
non-async too but right now bindgen can't differentiate between the fs write
function and the io write
function
ah, I was just figuring that out. These names aren't qualified, so we have some collisions.
I can work on improving that tomorrow
As a quick heads up, with the bindings cleanup patch, and moving filesystem functions into the resource impl:
error[E0195]: lifetime parameters or bounds on method `advise` do not match the trait declaration
--> crates/wasi/src/preview2/host/filesystem.rs:33:18
|
31 | #[async_trait::async_trait]
| --------------------------- this `where` clause might not match the one in the trait
32 | impl<T: WasiView> HostDescriptor for T {
33 | async fn advise(
| __________________^
34 | | &mut self,
35 | | fd: Resource<types::Descriptor>,
36 | | offset: types::Filesize,
37 | | len: types::Filesize,
38 | | advice: types::Advice,
39 | | ) -> Result<(), types::Error> {
| |_________^ lifetimes do not match method in trait
|
::: crates/wasi/src/preview2/mod.rs:143:7
|
143 | });
| - lifetimes in impl do not match this method in trait
It seems to be expecting advise
and all the other functions to be sync, even though they're in the list of async fns
oh I think it's because the methods changed
you'll probably need them to be [method]descriptor.advise
e.g. "[method]descriptor.advise"
Where do I use that syntax?
Oh, in the only_imports
list. That works.
ah yeah
oh hey that'd solve the namespacing issue too heh
albeit indirectly
sweet, I got all of the HostDescriptor
impl compiling
All of wasmtime-wasi now compiles!
It appears that wasmtime-wasi-http is pulling in the wasmtime-wasi bindings for polling and streams. Does it make sense for me to jump in and port that code to the new poll interfaces?
I'd naively say that whatever passes CI and gets WASI on resources is good enough for now
but that's without knowing many specifics
wasmtime-wasi-http doesn't compile right now and it looks like the easiest thing might be for me to just upgrade its poll and streams to resources too, so I'm now doing that
I'm reading over the PR to early-review
I want to make a cleanup pass and hopefully find some way to factor out all those Resource::new_borrow(fd.rep())
things, but it should be in a readable state.
yeah I was seeing that -- that's because there's two Descriptor
types for example, right?
one async and one sync?
Yeah, that's one big source of them
kk I can try to fix that with more features in bindgen!
The other is that Resource
doesn't have Clone
and sometimes we need to pass the same borrowed resource to multiple calls that need Resource
(not for this PR but for later)
ok intersting, I hesitate to add Clone
as it doesn't seem obviously correct, but it also doesn't seem obviously wrong
Yeah, same
This is an example of that right?
let input_stream = self
.table_mut()
.push_input_stream_child(input, Resource::<tcp::TcpSocket>::new_borrow(this.rep()))?;
let output_stream = self
.table_mut()
.push_output_stream_child(output, Resource::<tcp::TcpSocket>::new_borrow(this.rep()))?;
yeah
kk
that's tricky b/c we're probably not managing the actual table entry there?
actually, lemme read up a bit on this
so question for you, finish-connect: func() -> result<tuple<input-stream, output-stream>, error-code>
-- is the intent that finish-connect
sort of "consumes self"?
No, the socket handle remains live after finish-connect
, users need it to do getsockopt/setsockopt-like things
mk, but I think it may be a bug still then, where if you call that and then close the original socket?
hm I need to read more on .push_input_stream_child
hm ok this may need to get updated over time
this is the child/parent relationships where you can't destroy the parent if the child is still alive
I know we intentionally did this for pollable
which is more-or-less a child-like resource though
it's sort of beyond component-model semantics but that's not the end of the world
When we designed the API, we imagined they'd have reference-counting-like behavior, where you could drop the socket handle and keep the streams live.
yeah that seems more appropriate for sockets
But right now I'm just trying to work within the existing push_input_stream_child
behavior.
nah yeah that's fine
we may want to change the drop_*
methods which delete from the table to reference count in the table itself
so for example if you connect and then drop the socket that's ok, but the table entry is kept alive until the two other streams are also dropped and then it's finally actually removed from the table
I think we have all the hooks to implement that and shouldn't be too bad (no need to do it in this PR of course)
Ok, I did some porting of wasi-http but I'm encountering stuff that I don't know what to do with, so I'm going to ask around for help on those parts.
@Dan Gohman , i believe you can ignore error coming from the legacy modules (this file) as those will be completely removed really soon.
Oh, awesome. That should clear up a lot of it.
this change looks really great so far, all of the host changes are really simple and mechanical
updating the preview1 adapter is... complex. Handles aren't cloneable or even dupable, so a lot of adapter code that just throws around u32
code needs to be restructured
One option could be to add an off-by-default option to the rust generator to add Copy
and remove the Drop
implementation to assist with this
arg, so close
Caused by:
unsupported section found in adapter module', crates/test-programs/build.rs:232:10
:upside_down:
@Alex Crichton I'm seeing crates/test-programs/wasi-tests/src/bin/sleep.rs fail with "unknown handle index 1048096', crates/test-programs/tests/wasi-preview2-components.rs:284:30". 1048096 happens to be the address of the Pollable
in the guest. It looks like a disagreement about the lowering of list<borrow<Pollable>>
between the Rust guest bindings where it's an array of &Pollable
and the host bindings which appear to want an array of Pollable
.
got a command to repro?
definitely sounds like a bindgen issue
on the PR branch, cargo test --features "test-programs/test_programs" --package test-programs sleep
hm ok I get a failure
not unknown handle index
though
Do you have my latest branch changes?
yeah
er, 6dea64a708b25bb1c29216e050ddfeb512d3af29
running 1 test
calling subscribe
[crates/test-programs/wasi-tests/src/bin/sleep.rs:14] &p as *const _ = 0x000ffe20
[crates/test-programs/wasi-tests/src/bin/sleep.rs:16] list.as_ptr() = 0x000ffea8
calling poll
test sleep ... FAILED
failures:
---- sleep stdout ----
preopen: TempDir { path: "/var/folders/70/cx6_8hz93jz7kj_km0_5c9_40000gn/T/wasi_components_sleep_ZBEXxT" }
thread 'sleep' panicked at 'called `Result::unwrap()` on an `Err` value: error while executing at wasm backtrace:
0: 0x217824 - wit-component:shim!indirect-wasi:io/poll-poll-list
1: 0x137a - sleep::wasi::io::poll::poll_list::h0c3ff0fbc74b0bb0
at /Users/alex/code/wasmtime/crates/test-programs/wasi-tests/src/bin/sleep.rs:3:1
2: 0x1a68 - sleep::main::h05f297b3af9f1f16
at /Users/alex/code/wasmtime/crates/test-programs/wasi-tests/src/bin/sleep.rs:18:5
3: 0x1ce4 - core::ops::function::FnOnce::call_once::h59e587ee50bf110f
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/ops/function.rs:250:5
4: 0x1110 - std::sys_common::backtrace::__rust_begin_short_backtrace::h68fc03414118ad34
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/sys_common/backtrace.rs:135:18
5: 0xefe - std::rt::lang_start::{{closure}}::h30274f7edf3867b2
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/rt.rs:166:18
6: 0x3b3b - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h644be9b9b3cfec3d
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/ops/function.rs:284:13 - std::panicking::try::do_call::h1664f3cfdc3cc4ba
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:500:40 - std::panicking::try::h92522218794dbe07
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:464:19 - std::panic::catch_unwind::h09082efce4044946
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panic.rs:142:14 - std::rt::lang_start_internal::{{closure}}::h96d2e1cd5d5fb964
error: test failed, to rerun pass `-p test-programs --test wasi-preview2-components`
that's all I'm seeing
something looks suppressed by accident
I think stdout is being switched to nonblocking
b/c I saw a panic about that
huh, I'm not sure what's happening
intermittently I see
running 1 test
calling subscribe
[crates/test-programs/wasi-tests/src/bin/sleep.rs:14] &p as *const _ = 0x000ffe20
[crates/test-programs/wasi-tests/src/bin/sleep.rs:16] list.as_ptr() = 0x000ffea8
calling poll
test sleep ... FAILED
failures:
---- sleep stdout ----
preopen: TempDir { path: "/var/folders/70/cx6_8hz93jz7kj_km0_5c9_40000gn/T/wasi_components_sleep_7s0B4k" }
thread 'sleep' panicked at 'called `Result::unwrap()` on an `Err` value: error while executing at wasm backtrace:
0: 0x217824 - wit-component:shim!indirect-wasi:io/poll-poll-list
1: 0x137a - sleep::wasi::io::poll::poll_list::h0c3ff0fbc74b0bb0
at /Users/alex/code/wasmtime/crates/test-programs/wasi-tests/src/bin/sleep.rs:3:1
2: 0x1a68 - sleep::main::h05f297b3af9f1f16
at /Users/alex/code/wasmtime/crates/test-programs/wasi-tests/src/bin/sleep.rs:18:5
3: 0x1ce4 - core::ops::function::FnOnce::call_once::h59e587ee50bf110f
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/ops/function.rs:250:5
4: 0x1110 - std::sys_common::backtrace::__rust_begin_short_backtrace::h68fc03414118ad34
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/sys_common/backerror: io error when listing tests: Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }
trace.rs:135:18
5: 0xefe - std::rt::lang_start::{{closure}}::h30274f7edf3867b2
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/rt.rs:166:18
6: 0x3b3b - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h644be9b9b3cfec3d
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/ops/function.rs:284:13 - std::panicking::try::do_call::h1664f3cfdc3cc4ba
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:500:40 - std::panicking::try::h92522218794dbe07
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:464:19 - std::panic::catch_unwind::h09082efce4044946
at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panic.rs:1error: test failed, to rerun pass `-p test-programs --test wasi-preview2-components`
which is rust panicking about stdout being nonblocking I think
which would also explain the truncated error
I have enabled inherit_stdout
and inherit_stderr
for that test, just to be able to see the debug prints. Maybe the test is consuming the host process stdout and stderr.
diff --git a/crates/test-programs/tests/wasi-preview2-components.rs b/crates/test-programs/tes
ts/wasi-preview2-components.rs
index 7834562b6..1ff6a6ff5 100644
--- a/crates/test-programs/tests/wasi-preview2-components.rs
+++ b/crates/test-programs/tests/wasi-preview2-components.rs
@@ -281,7 +281,7 @@ async fn sched_yield() {
}
#[test_log::test(tokio::test(flavor = "multi_thread"))]
async fn sleep() {
- run("sleep", false).await.unwrap()
+ run("sleep", true).await.unwrap() // FIXME: temporarily enable stdio for debugging
}
#[test_log::test(tokio::test(flavor = "multi_thread"))]
async fn stdio() {
Maybe revert that part, and see if it fixes the test harness stdio?
hm no this appears to be macos specific
on Linux I get the full error with an unknown handle
I'll hunt down the macos bug later
ok yeah definitely a bug in bindgen
package foo:bar
world foo {
resource x
import y: func(a: list<borrow<x>>)
}
generates:
pub fn y(a: &[&X]) {
#[allow(unused_imports)]
use wit_bindgen::rt::{alloc, string::String, vec::Vec};
unsafe {
let vec0 = a;
let ptr0 = vec0.as_ptr() as i32;
let len0 = vec0.len() as i32;
#[cfg(target_arch = "wasm32")]
#[link(wasm_import_module = "$root")]
extern "C" {
#[link_name = "y"]
fn wit_import(_: i32, _: i32);
}
#[cfg(not(target_arch = "wasm32"))]
fn wit_import(_: i32, _: i32) {
unreachable!()
}
wit_import(ptr0, len0);
}
}
I'd hand-write the bindings for poll_list
for now instead
while I work on this
Ok, I can do that. The adapter already does this, so it's straightforward.
mind giving a thumbs up on https://github.com/bytecodealliance/wit-bindgen/pull/669? The fix for this will use some bits there
/me looks
:thumbs_up:
ok I think https://github.com/bytecodealliance/wit-bindgen/pull/670 fixes the issue here
although if possible I'd still recommend decoupling that PR from yours
and just leave a comment for it to get cleaned up later
Thanks!
Did you mention earlier that it might be possible to unify the resource types between the sync and async bindings? Right now, running commands on the wasmtime CLI on my branch fails because the stdio stream types now involve resources, and the wasmtime CLI uses the sync bindings, so it's expecting different resource types.
can't do that yet but I'm going to work on that next
ok, cool, yeah, no rush there
ok my truncated error message this morning was https://github.com/bytecodealliance/wasmtime/pull/7058
With my latest push, all of test-programs passes!
I compiled a wit-bindgen with bytecodealliance/wit-bindgen#670 and copied in the bindings, so I can confirm that that fixes the problem I was seeing.
nice
once that's in I can publish so it can get pulled in
ok wit-bindgen is now published
@Dan Gohman I've got some cleanups and various changes to the preview1 adapter, would you like me to PR them or would it be ok to push up to the PR?
or alternatively if you're working on this area I'll let you go gangbuster
I'm in the middle of trying to reorganize things to fix the &mut Descriptors
problem
ok I'll wait for that
I had a rough idea there which was to use &[RefCell<Descriptor>]
and then get_file
etc methods would return Result<Ref<'_, File>>
I didn't actually implement it fully though
Oh, interesting. That might be better than my idea, which was to make descriptors()
and descriptors_mut()
take a closure that gets passed the Descriptors
object.
yeah my thinking was that ideally we shy away from all unsafe
code and stick to libstd/libcore primitives if possible
That's obviously good in theory, but I keep trying to use RefCell
in various ways and it keeps not working :-}.
But your idea of pushing it down into the array might work out better.
nah yeah I know what you mean
How would the &[RefCell<Descriptor>]
case handle adding a new descriptor to the table?
oh I thought we already had unsafe bits for that
like a cap pointer which we bumped and ptr::write
-ed the new value into
ah yeah push
is already unsafe in that regard which I think is fine to keep
ok I've pushed up configuration of resources in https://github.com/bytecodealliance/wasmtime/pull/7069
Last updated: Dec 23 2024 at 14:03 UTC