add `wasm32-wasip2` support #1836

dicej · 2024-10-09T15:52:49Z

This implementation currently uses a mix of POSIX-style APIs (provided by wasi-libc via the libc crate) and WASIp2-native APIs (provided by the wasi crate).

Alternatively, we could implement Selector using only POSIX APIs, e.g. poll(2). However, that would add an extra layer of abstraction to support and debug, as well as make it impossible to support polling wasi:io/poll/pollable objects which cannot be represented as POSIX file descriptors (e.g. timer events, DNS queries, HTTP requests, etc.).

Another approach would be to use only the WASIp2 APIs and bypass wasi-libc entirely. However, that would break interoperability with both Rust std and e.g. C libraries which expect to work with file descriptors.

Since wasi-libc does not yet provide a public API for converting between file descriptors and WASIp2 resource handles, we currently use a non-public API (see the netc module below) to do so. Once
WebAssembly/wasi-libc#542 is addressed, we'll be able to switch to a public API.

I've tested this end-to-end using https://github.com/dicej/wasi-sockets-tests, which includes smoke tests for mio, tokio, tokio-postgres, etc.

This implementation currently uses a mix of POSIX-style APIs (provided by `wasi-libc` via the `libc` crate) and WASIp2-native APIs (provided by the `wasi` crate). Alternatively, we could implement `Selector` using only POSIX APIs, e.g. `poll(2)`. However, that would add an extra layer of abstraction to support and debug, as well as make it impossible to support polling `wasi:io/poll/pollable` objects which cannot be represented as POSIX file descriptors (e.g. timer events, DNS queries, HTTP requests, etc.). Another approach would be to use _only_ the WASIp2 APIs and bypass `wasi-libc` entirely. However, that would break interoperability with both Rust `std` and e.g. C libraries which expect to work with file descriptors. Since `wasi-libc` does not yet provide a public API for converting between file descriptors and WASIp2 resource handles, we currently use a non-public API (see the `netc` module below) to do so. Once WebAssembly/wasi-libc#542 is addressed, we'll be able to switch to a public API. I've tested this end-to-end using https://github.com/dicej/wasi-sockets-tests, which includes smoke tests for `mio`, `tokio`, `tokio-postgres`, etc. Signed-off-by: Joel Dice <[email protected]>

This adds support for the new `wasm32-wasip2` target platform, which includes more extensive support for sockets than `wasm32-wasip1` (formerly known as `wasm32-wasi`). The bulk of the changes are in tokio-rs/mio#1836. This patch just tweaks a few `cfg` directives to indicate `wasm32-wasip2`'s additional capabilities. In the future, we could consider adding support for `ToSocketAddrs`. WASIp2 natively supports asynchronous DNS lookups and is single threaded, whereas Tokio currently assumes DNS lookups are blocking and require multithreading to emulate async lookups. A WASIp2-specific implementation could do the lookup directly without multithreading. I've tested this end-to-end using https://github.com/dicej/wasi-sockets-tests, which includes smoke tests for `mio`, `tokio`, `tokio-postgres`, etc. I'd also be happy to add tests to this repo if appropriate; it would require adding a dev-dependency on e.g. `wasmtime` to actually run the test cases. Signed-off-by: Joel Dice <[email protected]>

dicej · 2024-10-09T17:32:29Z

I just realized this PR doesn't include support for accepting incoming connections -- only initiating outgoing ones. I forgot about the former since I don't personally have an urgent need to support it, but it could be added as a follow-up PR. To be clear: WASIp2 is fully capable of handling that case.

raskyld · 2024-10-09T18:37:29Z

src/sys/wasi/p2.rs

+                let mut push_event = || {
+                    events.push(Event {
+                        token: subscription.token,
+                        interests: *interests,
+                    })
+                };


Just out of curiosity, if there any reason to do that in a closure vs. doing it inline at L276, L289 and L297?

No particular reason other than avoiding the repetition.

Alright! Was not sure if it was DRY or there was a mechanism I am not aware of, thanks for clarification 🙏

raskyld · 2024-10-09T19:26:58Z

Also, I have a last clarifying question: reading wasi-libc I assumed that all Pollable are abstracted as a Socket but this socket is a "fake" one in the sense that we only use it to convey the "readiness" of the underlying Pollable irrespectively of whether it is actually obtained through wasi:sockets.

But your comment:

I just realized this PR doesn't include support for accepting incoming connections -- only initiating outgoing ones. I forgot about the former since I don't personally have an urgent need to support it, but it could be added as a follow-up PR. To be clear: WASIp2 is fully capable of handling that case.

Actually speak of connections, so I am not sure my previous assumption was correct.

Anyway, great job 💪

dicej · 2024-10-09T19:53:11Z

Also, I have a last clarifying question: reading wasi-libc I assumed that all Pollable are abstracted as a Socket but this socket is a "fake" one in the sense that we only use it to convey the "readiness" of the underlying Pollable irrespectively of whether it is actually obtained through wasi:sockets.

WASIp2 represents things like TCP and UDP sockets as resources (e.g. tcp-socket) which have methods for binding, listening, connecting, etc. Once a socket is connected, you can get its input-stream and output-stream, which are also resources. And each of those can be used to obtain a pollable representing read and write readiness, respectively.

Consequently, wasi-libc must keep track of up to six resource handles for each socket:

a tcp-socket or udp-socket, depending on the socket type
a pollable representing readiness of any in-progress bind, listen, connect, or accept operation
(if connected) an input-stream, an output-stream, and one pollable each for reading and writing

So you can think of a wasi-libc socket file descriptor as uniquely identifying a bundle of resource handles, the number and types of which depends on the state that socket is in.

Adding support for binding, listening and accepting WASIp2 sockets in mio would amount to adding match cases for e.g. tcp_socket_state_tag_t::TCP_SOCKET_STATE_UNBOUND, tcp_socket_state_tag_t::TCP_SOCKET_STATE_BOUND, and tcp_socket_state_tag_t::TCP_SOCKET_STATE_LISTENING and using the socket_pollable field of tcp_socket_t to await transitions to the next state.

Does that help? Happy to go into more detail if desired. Also, the wasi-sockets docs are quite thorough if you haven't perused them yet.

dicej · 2024-10-09T19:55:59Z

BTW, this PR doesn't include UDP support either, again because that hasn't been a priority for me. Shouldn't be hard to add as a follow-up PR.

raskyld · 2024-10-09T20:11:47Z

Thanks a lot for the time you took answering me 🙏
Your answer is super clear!

My question was more related to how pollable obtained from types not related to sockets are usable in mio context, for example, the streams you mentioned can also be obtained through the wasi:http world: https://github.com/WebAssembly/wasi-http/blob/main/wit/types.wit#L510

In this case, I suspect you still have the streams and the pollable but not the sockets resource handles.
Could you still use those resource handles to Poll them in mio? IIUC, the philosophy of the crate is to build upon any event source independently of the platform primitive under of that but I may misunderstand it!

dicej · 2024-10-09T20:27:48Z

My question was more related to how pollable obtained from types not related to sockets are usable in mio context, for example, the streams you mentioned can also be obtained through the wasi:http world: https://github.com/WebAssembly/wasi-http/blob/main/wit/types.wit#L510

Oh right, great question. Yeah, I think we'd need to add a new, WASIp2-only API for registering pollables that have no corresponding file descriptors (and likewise for tokio, presumably). This PR clearly doesn't include such a thing, but it's something we could add in another PR.

Another approach would be to add an API to wasi-libc that accepts an arbitrary pollable and allocates a file descriptor for it.

@badeend and @sunfishcode might have thoughts about this.

raskyld · 2024-10-09T20:53:42Z

Alright thanks for the answer! Reading WebAssembly/wasi-libc#542 it seems to me that the question of allocating fd to pollable was among the initial options, I wonder though how would you implement stuff like fstat for those fd 🤔 but let's not pollute your MR!

To get back to the main subject,
I just remembered I wondered why you use atomic types and mutex in the PR since wasip2 is single-threaded?
Is it because of the thread proposal ?

dicej · 2024-10-09T21:12:29Z

To get back to the main subject, I just remembered I wondered why you use atomic types and mutex in the PR since wasip2 is single-threaded? Is it because of the thread proposal ?

Honestly, I just copied that from the existing WASIp1 implementation. I wrote this code almost a year ago and only came back to it yesterday, so it's not super fresh in my mind, but that part at least came straight from the existing code. I'm guessing the original code either needed it to make the compiler happy (e.g. make Selector Send and Sync) or for future-proofing. Arc and Mutex are roughly equivalent to Rc and RefCell on single-threaded Wasm, anyway, so there shouldn't be a performance penalty.

Darksonn · 2024-10-10T11:04:27Z

Cargo.toml

+[target.'cfg(all(target_os = "wasi", not(target_env = "p2")))'.dependencies]
 wasi = "0.11.0"
+
+[target.'cfg(all(target_os = "wasi", target_env = "p2"))'.dependencies]
+wasi = "0.13.3"


Is this really the correct way to go about this? Only 0.11 supports p1?

Looking at https://docs.rs/wasi/latest/wasi/
I would say yes, the last mention I see of p1 is on 0.11

Correct; see https://docs.rs/wasi/latest/wasi/#wasi-today-and-wasi_snapshot_preview1

Darksonn · 2024-10-10T11:06:37Z

src/io_source.rs

@@ -175,7 +175,7 @@ where
    }
 }

-#[cfg(target_os = "wasi")]
+#[cfg(all(target_os = "wasi", not(target_env = "p2")))]


Can we do this?

Suggested change

#[cfg(all(target_os = "wasi", not(target_env = "p2")))]

#[cfg(all(target_os = "wasi", target_env = "p1"))]

The reason I did it that way is because not(target_env = "p2") is backwards compatible with rustc versions prior to the introduction of the wasm32-wasip2 target (i.e. all stable rustc versions as of this writing). See here for further discussion. It does mean we'll need to make changes when p3 becomes available, though. Happy to do whatever you think is best here.

Darksonn · 2024-10-10T11:07:14Z

src/net/tcp/stream.rs

@@ -87,10 +87,10 @@ impl TcpStream {
    /// entries in the routing cache.
    ///
    /// [write interest]: Interest::WRITABLE
-    #[cfg(not(target_os = "wasi"))]
+    #[cfg(any(not(target_os = "wasi"), target_env = "p2"))]


Can you use not(all(...)) here?

If you mean not(all(target_os = "wasi", target_env = "p1")) then the same rationale I gave above for not using target_env = "p1" applies. Again, though, happy to change it if we're not concerned about backwards rustc compatibility.

raskyld · 2024-10-10T12:05:27Z

After a good night of sleep, I realised the name of the PR should probably mention that it only adds support for established wasi:sockets so it's clear that we will need follow-up PRs for other pollable and for initiating connections.

WDYT?

dicej · 2024-10-10T13:36:38Z

After a good night of sleep, I realised the name of the PR should probably mention that it only adds support for established wasi:sockets so it's clear that we will need follow-up PRs for other pollable and for initiating connections.

WDYT?

Makes sense; I'll add some TODO comments to the code and to the commit message.

// TODO tokio-rs#1: Add a public, WASIp2-only API for registering // `wasi::io::poll::Pollable`s directly (i.e. those which do not correspond to // any `wasi-libc` file descriptor, such as `wasi:http` requests). // // TODO tokio-rs#2: Add support for binding, listening, and accepting. This would // involve adding cases for `TCP_SOCKET_STATE_UNBOUND`, // `TCP_SOCKET_STATE_BOUND`, and `TCP_SOCKET_STATE_LISTENING` to the `match` // statements in `Selector::select`. // // TODO tokio-rs#3: Add support for UDP sockets. This would involve adding cases for // the `UDP_SOCKET_STATE_*` tags to the `match` statements in // `Selector::select`. Signed-off-by: Joel Dice <[email protected]>

Thomasdezeeuw

I don't really have much time to review this.

Can we split this up in multiple prs as this seems to do multiple things.

A minimal pr that adds support for v2, no adding of v2 functionality.
The selector rewrite (not sure why this is needed)
Any v2 additional we make, such as support for more API

Thomasdezeeuw · 2024-10-14T16:07:27Z

src/sys/wasi/p2.rs

+        let mut subscriptions = self.subscriptions.lock().unwrap();
+
+        let mut states = Vec::new();
+        for (fd, subscription) in subscriptions.deref() {


I don't think looping of all active subscriptions is a great idea. Nor is the allocation for states.

Why switch from a Vec to a HashMap?

I'm happy to optimize by reducing the number of allocations if desired. Would you prefer that I update this PR or leave that for a follow-up PR?

Why switch from a Vec to a HashMap?

I was aiming for O(1) lookups by file descriptor in the IoSourceState implementation.

I'm happy to optimize by reducing the number of allocations if desired. Would you prefer that I update this PR or leave that for a follow-up PR?

Why switch from a Vec to a HashMap?

I was aiming for O(1) lookups by file descriptor in the IoSourceState implementation.

It might be slower when taking into account hashing tho 🤔

It might be slower when taking into account hashing tho 🤔

Possibly; I can switch back to using a Vec if preferred.

dicej · 2024-10-14T17:42:59Z

Can we split this up in multiple prs as this seems to do multiple things.

1. A minimal pr that adds support for v2, no adding of v2 functionality.

2. The selector rewrite (not sure why this is needed)

3. Any v2 additional we make, such as support for more API

Items 1 and 2 are intertwined. None of the WASIp1 APIs are available in WASIp2, so a new selector implementation is needed to target the new APIs. There is an adapter which implements (most of) the WASIp1 APIs in terms of their WASIp2 counterparts, but wasi-libc doesn't use the adapter for sockets -- it uses the WASIp2 APIs directly in order to access the much broader socket support that WASIp2 provides.

As I mentioned above, we do have a few different options for a WASIp2 selector implementation (use POSIX poll(2), use the WASIp2 APIs directly, or use a mix of POSIX file descriptors and direct WASIp2 APIs for maximum compatibility), but reusing the WASIp1 implementation is not one of those options.

Totally agreed that we can leave item 3 in your list for a later PR, though.

jeffparsons · 2024-10-25T21:24:23Z

Just to clarify, is this currently "waiting-on-review", to use the rust-lang parlance? Or, conversely, is further review currently blocked on any action from the author?

Thomasdezeeuw · 2024-10-26T10:09:56Z

@jeffparsons this is waiting on an action on #1836 (review). We currently don't have the capacity to review a pr of this size, so it needs to be split up.

jeffparsons · 2024-10-27T04:43:14Z

@jeffparsons this is waiting on an action on #1836 (review). We currently don't have the capacity to review a pr of this size, so it needs to be split up.

Thanks, @Thomasdezeeuw. I wonder if that was perhaps not clear to @dicej because his later comment suggests there's no natural decomposition of what's in this PR, which gave me the impression that he was waiting for feedback on his explanation.

Just floating this idea: would it be possible/acceptable to both author and reviewer to split this into at least:

just common plumbing (e.g. selector) without any actual socket APIs
outgoing socket APIs

Would that make review more manageable? Are there other axes this could be reasonably split on?

I'm just an observer, so sorry if this isn't helpful. Just trying to see if there's anything that could get this out of limbo.

💖

dicej · 2024-10-28T14:12:36Z

@jeffparsons this is waiting on an action on #1836 (review). We currently don't have the capacity to review a pr of this size, so it needs to be split up.

Along the lines of what @jeffparsons suggested, I can definitely split this up if we're not concerned about delivering useful functionality to wasm32-wasip2 users right off the bat. For example, I could start with a minimal scaffolding for the new selector that compiles but with only todo!() in the function bodies, then reintroduce the rest of the implementation incrementally until we have something that's actually useful. How does that sound?

jeffparsons · 2024-11-07T01:44:10Z

@Thomasdezeeuw Does the proposal in #1836 (comment) sound okay to you?

Thomasdezeeuw

I had a little time to look into this and TL;DR it's a no-go from me.

With the changes to wasi crate it seems to have lost the raw system (runtime) call layer. poll_oneoff seems to have been changed to poll, except that wasi now allocates for the result, which we do not want.

Specific to this pr, the netc module with all the type definitions will not be accepted. Similar to other OS I do not want to maintain the definitions, especially not one for a specification that is still in flux. This should live in a libc-like crate, which I would expect to be wasi but I'm not sure after having (quickly) looked at v0.13.

Also specific to this pr is the actual implementation. Looking at wasi-p2 to me it seems we can change poll_oneoff to poll and it should mostly work (though I would expect certain edge cases). I don't understand why we need this many changes for this.

I think these are the three main blockers at the moment. But I didn't do a full review.

dicej · 2024-11-13T19:21:49Z

Thanks for the feedback, @Thomasdezeeuw !

poll_oneoff seems to have been changed to poll, except that wasi now allocates for the result, which we do not want.

That's an artifact of how WASIp2's poll function is defined, along with the underlying ABI, which determines how the host allocates and returns a list<u32> from a function. Specifically, the host calls a cabi_realloc function exported by the guest to do the allocation.

It's possible to optimize that by pre-allocating an conservatively-sized buffer before calling poll, storing a pointer to it in a global, and then returning that pointer when the host calls cabi_realloc. Then we could reuse that buffer across Selector::select calls, eliminating any per-call allocation overhead. The downside is that we'd have to write that code by hand rather than use the auto-generated bindings from the wasi crate, but maybe it's worth it in this case?

Specific to this pr, the netc module with all the type definitions will not be accepted.

That's fair. Seems like libc would be a good home for those.

Looking at wasi-p2 to me it seems we can change poll_oneoff to poll and it should mostly work (though I would expect certain edge cases).

The main difference between the WASIp1 and WASIp2 APIs is that the latter doesn't have any types corresponding to p1's subscription and eventtype types, etc. In p1, those types allowed the caller to tell poll_oneoff which events were of interest (in the case of subcription) and poll_oneoff to tell the caller which events actually occurred (in the case of eventtype). In p2, we must create and pass a separate pollable handle to poll for each event of interest.

In the case of p2, the pollable type is opaque, so we need to remember which socket (or other poll-able thing) and operation (e.g. read, write, or accept) that pollable represents, which accounts for some of the differences between p1.rs and p2.rs. The other differences are related to the fact that p2 simply supports more things (e.g. making outgoing connections) than p1 did.

That said, I definitely hear what you're saying about not wanting to review or maintain a lot of new code. With that in mind, we could come at this from a completely different direction, per the code comment I put in p2.rs:

// Alternatively, we could implement Selector using only POSIX APIs,
// e.g. poll(2). However, that would add an extra layer of abstraction to
// support and debug, as well as make it impossible to support polling
// wasi:io/poll/pollable objects which cannot be represented as POSIX file
// descriptors (e.g. timer events, DNS queries, HTTP requests, etc.).

Although I am a bit concerned about the extra level of abstraction, simply reusing src/sys/unix/selector/poll.rs for WASIp2 certainly has some appeal. Then we'd let wasi-libc take care of the details of abstracting the WASIp2 interface in terms of the POSIX poll(2) interface (e.g. wrangling pollables and keeping track of what they correspond to). And despite what I said in that comment, we could still potentially support polling other poll-able things like HTTP requests using this approach.

@Thomasdezeeuw, what do you think of the following as a path forward?

I close this PR
Someone (maybe me or someone else if they beat me to it) tests to verify that src/sys/unix/selector/poll.rs works correctly on WASIp2 and opens a PR to use that instead of src/sys/wasi/mod.rs when building for wasm32-wasip2, plus whatever other minor changes are needed (e.g. the tweaks I made to src/net/tcp/stream.rs in this PR)

This was referenced Oct 9, 2024

add wasm32-wasip2 support tokio-rs/tokio#6893

Draft

WASI support #1346

Closed

meta: Stabilize WASI support tokio-rs/tokio#4827

Open

raskyld reviewed Oct 9, 2024

View reviewed changes

Darksonn reviewed Oct 10, 2024

View reviewed changes

Thomasdezeeuw requested changes Oct 14, 2024

View reviewed changes

raskyld mentioned this pull request Oct 18, 2024

Makes the wasm32-wasip1/2 target a first-class citizen for Leptos's Server-Side leptos-rs/leptos#3063

Merged

8 tasks

Thomasdezeeuw requested changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `wasm32-wasip2` support #1836

add `wasm32-wasip2` support #1836

dicej commented Oct 9, 2024

dicej commented Oct 9, 2024

raskyld Oct 9, 2024

dicej Oct 9, 2024

raskyld Oct 9, 2024

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024 •

edited

Loading

dicej commented Oct 9, 2024

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024

Darksonn Oct 10, 2024

raskyld Oct 10, 2024

dicej Oct 10, 2024

Darksonn Oct 10, 2024

dicej Oct 10, 2024

Darksonn Oct 10, 2024

dicej Oct 10, 2024 •

edited

Loading

raskyld commented Oct 10, 2024 •

edited

Loading

dicej commented Oct 10, 2024

Thomasdezeeuw left a comment

Thomasdezeeuw Oct 14, 2024

dicej Oct 14, 2024

banocean Oct 16, 2024

dicej Oct 16, 2024

dicej commented Oct 14, 2024

jeffparsons commented Oct 25, 2024

Thomasdezeeuw commented Oct 26, 2024

jeffparsons commented Oct 27, 2024

dicej commented Oct 28, 2024

jeffparsons commented Nov 7, 2024

Thomasdezeeuw left a comment

dicej commented Nov 13, 2024

	#[cfg(all(target_os = "wasi", not(target_env = "p2")))]
	#[cfg(all(target_os = "wasi", target_env = "p1"))]

add wasm32-wasip2 support #1836

Are you sure you want to change the base?

add wasm32-wasip2 support #1836

Conversation

dicej commented Oct 9, 2024

dicej commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024 • edited Loading

dicej commented Oct 9, 2024

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024

raskyld commented Oct 9, 2024

dicej commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dicej Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

raskyld commented Oct 10, 2024 • edited Loading

dicej commented Oct 10, 2024

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dicej commented Oct 14, 2024

jeffparsons commented Oct 25, 2024

Thomasdezeeuw commented Oct 26, 2024

jeffparsons commented Oct 27, 2024

dicej commented Oct 28, 2024

jeffparsons commented Nov 7, 2024

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

dicej commented Nov 13, 2024

add `wasm32-wasip2` support #1836

add `wasm32-wasip2` support #1836

dicej commented Oct 9, 2024 •

edited

Loading

dicej Oct 10, 2024 •

edited

Loading

raskyld commented Oct 10, 2024 •

edited

Loading