Refactor futures/streams support for Rust by alexcrichton · Pull Request #1249 · bytecodealliance/wit-bindgen

alexcrichton · 2025-03-28T21:41:14Z

Mostly remove the old implementations for futures/streams and replace them with new versions which address a number of preexisting issues and additionally adapt to upcoming ABI changes.

Closes #1152 - there's now a more custom interface for streams which doesn't use sink/stream traits
Closes #1174 - error-context has been removed from futures/streams
Closes #1177 - all "core" futures are now manual state machines, not async fn
Closes #1178 - lowering/lifting was adjusted to avoid leaking anything
Closes #1180 - this should now work
Closes #1181 - the "local" case will trap now in the component model ABI
Closes #1182 - dummy wakers haven't been used for awhile, doubly so here
Closes #1184 - futures/streams are reader/writer pairs with much simpler ownership
Closes #1188 - the intrinsic return value is now handled and accounts for the operation succeeding racily
Closes #1189 - error-context integration with streams/futures is removed
Closes #1190 - Stream{Writer,Reader} take and give buffers, no allocations for stream<u8>
Closes #1191 - FutureWriter::write::cancel returns Result<(), FutureWriteCancel<T>>

alexcrichton · 2025-03-28T21:42:22Z

This is a bit of a work-in-progress, but opening this up for upstream PRs. Will flesh out more in the future

Mostly remove the old implementations for futures/streams and replace them with new versions which address a number of preexisting issues and additionally adapt to upcoming ABI changes.

alexcrichton · 2025-03-28T22:02:44Z

crates/guest-rust/rt/src/async_support.rs

+        EVENT_STREAM_READ | EVENT_STREAM_WRITE | EVENT_FUTURE_READ | EVENT_FUTURE_WRITE => {
+            (*state).remove_waitable(event1 as u32);
+            let (waker, code) = (*state).wakers.remove(&(event1 as u32)).unwrap();
+            *code = Some(event2 as u32);
+            waker.wake();
+
+            let done = poll(state).is_ready();
+            callback_code(state, done)
+        }


I'll note that in the future I want to fold EVENT_CALL_RETURNED into this exact same pattern as well, but that'll require some refactoring to achieve that.

crates/guest-rust/rt/src/async_support/abi_buffer.rs

crates/guest-rust/rt/src/async_support/future_support.rs

crates/guest-rust/rt/src/async_support/stream_support.rs

alexcrichton · 2025-03-28T22:06:36Z

crates/guest-rust/rt/src/lib.rs

+
+/// Cleanup helper used to deallocate blocks of canonical ABI data from
+/// lowerings.
+pub struct Cleanup {


I'll note that adding this is going to wreak havoc with cargo component. I don't know what to do about that.

Ok I have now articulated a long-term goal: bytecodealliance/cargo-component#387

Now the question is what to do about this in the short term.

Ok did some thinking (aka paused and did a think) -- I think we should go ahead and land this refactoring. This means that wit-bindgen basically can't get updated in cargo-component without breaking things, but it won't break anything itself. Not being able to update for a bit is I think probably the best tradeoff here for now.

alexcrichton · 2025-03-28T22:08:54Z

crates/rust/src/interface.rs

    }
 }
-
-fn stream_direct(ty: &Type) -> bool {


I've replaced this with is_list_canonical which already existed but was somewhere else, so I refactored it to be available here.

tests/runtime-new-async-disabled/future-cancel-write-then-read/runner.rs

Require source edits for now though to enable it.

alexcrichton · 2025-03-31T18:26:15Z

Ok @dicej I've thought through all the outstanding an open issues (sorry about the torrent of notifications here) and this should be good-to-go.

dicej · 2025-03-31T18:31:40Z

crates/guest-rust/rt/src/async_support/abi_buffer.rs

+            let len = vec.len();
+            let cap = vec.capacity();
+            mem::forget(vec);
+            Vec::<MaybeUninit<T>>::from_raw_parts(ptr.cast(), len, cap)


Out of curiosity, what's the advantage of doing it this way vs. using transmute? Are there soundness considerations?

AFAIK this is a situation where it works and probably will work for a long time but everyone will still frown at you for doing it, so I've opted to be a bit safer here and sidestep the question of "is it ok?"

crates/guest-rust/rt/src/async_support/abi_buffer.rs

dicej · 2025-03-31T19:11:53Z

crates/guest-rust/rt/src/async_support/future_support.rs

+    ///
+    /// Panics if the operation has already been completed via `Future::poll`,
+    /// or if this method is called twice.
+    pub fn cancel(self: Pin<&mut Self>) -> Result<(), FutureWriteCancel<T>> {


What's the rationale for using Result here rather than adding a FutureWriteCancel::AlreadyDelivered variant and having this return FutureWriteCancel<T> instead? It's a little surprising to me that we'd consider FutureWriteCancel::Cancelled (or really any of the possible outcomes) to be an error.

Originally I wanted to avoid making a new error type, but then when I ended up making a new error type anyway I didn't actually get back to fixing this. I'll update that. I think I'll probably just return an enum here instead of a result after all

crates/guest-rust/rt/src/async_support/stream_support.rs

dicej · 2025-03-31T19:27:53Z

crates/guest-rust/rt/src/async_support/stream_support.rs

+    /// `Some(value)`, otherwise `None` is returned indicating the value was
+    /// sent.
+    pub async fn write_one(&mut self, value: T) -> Option<T> {
+        // TODO: can probably be a bit more efficient about this and avoid


FWIW, this is how I addressed it on the host side, i.e. by making the buffer type generic and implementing it for Vec, Bytes, and Single. That would require significant refactoring here, though, given the way AbiBuffer is currently tied to Vec.

Good point yeah, I kind of want to avoid the complexity for now but I do still want to leave a note about a potential future optimization, so I'm probably going to leave this as is (and might advocate for the host to do the same in the near term...)

To be clear, the host optimization was primarily motivated by being able to support bytes::Bytes (which is what Hyper uses out of the box, and thus wasmtime-wasi-http) natively without any conversions. The single-element optimization was just a nice side-effect of generic-izing the code.

crates/guest-rust/rt/src/async_support/waitable.rs

Refactor futures/streams support for Rust

4c89fc0

Mostly remove the old implementations for futures/streams and replace them with new versions which address a number of preexisting issues and additionally adapt to upcoming ABI changes.

alexcrichton force-pushed the wit-bindgen-async-tests branch from f213e81 to 4c89fc0 Compare March 28, 2025 21:52

alexcrichton added 3 commits March 28, 2025 14:59

Don't run new tests just yet

4752ef9

Go back to original wac deps

c60bbc4

Disable patch overrides

5bcc78e

alexcrichton commented Mar 28, 2025

View reviewed changes

alexcrichton mentioned this pull request Mar 28, 2025

Updating for wit-bindgen refactoring bytecodealliance/wasip3-prototyping#94

Merged

alexcrichton added 8 commits March 28, 2025 18:34

Beef up some documentation in various places

07f7ecb

Remove some dead code

c6ec1e8

Defer async tests to a future PR

eb1f4b7

Add some tests to AbiBuffer<T>

4e65cf4

Try to fix CI

65cebe9

Add miri too

44fec39

Add some built-in debugging support

917a2e2

Require source edits for now though to enable it.

Document CompletionStatus

27d673a

alexcrichton mentioned this pull request Mar 31, 2025

Generate WIT, not Rust source code bytecodealliance/cargo-component#387

Open

alexcrichton marked this pull request as ready for review March 31, 2025 18:25

dicej approved these changes Mar 31, 2025

View reviewed changes

Review comments

271a047

alexcrichton added this pull request to the merge queue Mar 31, 2025

Merged via the queue into bytecodealliance:main with commit 40ca9b1 Mar 31, 2025
18 checks passed

alexcrichton deleted the wit-bindgen-async-tests branch March 31, 2025 22:20

Conversation

alexcrichton commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexcrichton commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexcrichton commented Mar 28, 2025 •

edited

Loading