wasmtime / PR #11515 Revamp component model stream/future... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11515 Revamp component model stream/future...

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:42):

dicej opened PR #11515 from dicej:stream-future-api-revamp to bytecodealliance:main:

This changes the host APIs for dealing with futures and streams from a "rendezvous"-style API to a callback-oriented one.

Previously you would create e.g. a StreamReader/StreamWriter pair and call their read and write methods, respectively, and those methods would return Futures that resolved when the operation was matched with a corresponding write or read operation on the other end.

With the new API, you instead provide a StreamProducer trait implementation whe creating the stream, whose produce method will be called as soon as a read happens, giving the implementation a chance to respond immediately without making the reader wait for a rendezvous. Likewise, you can match the read end of a stream to a StreamConsumer to respond immediately to writes. This model should reduce scheduling overhead and make it easier to e.g. pipe items to/from AsyncWrite/AsyncRead or Sink/Stream implementations without needing to explicitly spawn background tasks. In addition, the new API provides direct access to guest read and write buffers for stream<u8> operations, enabling zero-copy operations.

Other changes:

I've removed the HostTaskOutput; we were using it to run extra code with access to the store after a host task completes, but we can do that more elegantly inside the future using tls::get. This also allowed me to simplify Instance::poll_until a bit.

I've removed the watch_{reader,writer} functionality; it's not needed now given that the runtime will automatically dispose of the producer or consumer when the other end of the stream or future is closed -- no need for embedder code to manage that.

In order to make UntypedWriteBuffer Send, I had to wrap its raw pointer buf field in a SendSyncPtr.

I've removed {Future,Stream}Writer entirely and moved Instance::{future,stream} to {Future,Stream}Reader::new, respectively.

I've added a bounds check to the beginnings of Instance::guest_read and Instance::guest_write so that we need not do it later in Guest{Source,Destination}::remaining, meaning those functions can be infallible.

Note that I haven't updated wasmtime-wasi yet to match; that will happen in one or more follow-up commits.

Wasmtime GitHub notifications bot (Aug 23 2025 at 00:58):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Aug 23 2025 at 00:59):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Aug 25 2025 at 15:23):

alexcrichton created PR review comment:

Technically this should use poll_ready, right?

Wasmtime GitHub notifications bot (Aug 25 2025 at 15:23):

alexcrichton submitted PR review:

Is it worth it to resolve the zero-length read/write TODOs in the code before landing? We don't have many users of that right now so it also seems ok to defer that too.

Otherwise though this all seems reasonable to me, although I'm mostly relying on tests. The interfaces we've talked about historically and I think are ok to land. I'm also happy to help out with the porting of wasmtime-wasi later this afternoon

Wasmtime GitHub notifications bot (Aug 25 2025 at 15:23):

alexcrichton created PR review comment:

Technically this should call .next() and buffer up the result if one comes in, right?

Wasmtime GitHub notifications bot (Aug 25 2025 at 16:24):

dicej commented on PR #11515:

Is it worth it to resolve the zero-length read/write TODOs in the code before landing? We don't have many users of that right now so it also seems ok to defer that too.

Yup, I'll do that. Testing it will be mildly tedious, but might as well bite the bullet.

Otherwise though this all seems reasonable to me, although I'm mostly relying on tests. The interfaces we've talked about historically and I think are ok to land. I'm also happy to help out with the porting of wasmtime-wasi later this afternoon

@rvolosatovs is planning to take a crack at updating wasmtime-wasi starting tomorrow, FYI.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:38):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:38):

rvolosatovs created PR review comment:

It appears that we'll need to expose the second type parameter to Accessor in order to be able to use this API from within e.g. wasmtime_wasi.
Binding implementations get an opaque T passed in, e.g. https://github.com/bytecodealliance/wasmtime/blob/3aa392399e785df414ad6ee6ee7f034989b75301/crates/wasi/src/p3/sockets/host/types/tcp.rs#L252-L253
pub trait FutureProducer<T, U, V>: Send + 'static {
    /// Handle a host- or guest-initiated read by producing a value.
    fn produce(self, accessor: &Accessor<T, U>) -> impl Future<Output = Result<V>> + Send;
}

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:41):

alexcrichton created PR review comment:

Oh for that @dicej and I talked about this a bit ago, I'd prefer to avoid the type parameter explosion but I have an alternative solution that should work. Let me sketch that out and push it up here too

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:49):

alexcrichton updated PR #11515.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:49):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:49):

rvolosatovs created PR review comment:

Awesome, thanks @alexcrichton .
I actually just ran into a very similar issue earlier today in wasi:http, trying to reuse the same task implementation both from outside and from within the wasi:http crate, which I worked around the following way:

impl<T, U, Fut> AccessorTask<T, U, wasmtime::Result<()>> for GuestBodyTask<Fut>
where
    T: WasiHttpView,
    U: HasData,
    Fut: Future<Output = Result<(), ErrorCode>> + Send + 'static,
{
    async fn run(self, store: &Accessor<T, U>) -> wasmtime::Result<()> {
        self.run(store, |mut store, trailers| {
            store
                .data_mut()
                .http()
                .table
                .delete(trailers)
                .context("failed to delete trailers")
        })
        .await
    }
}

/// This is a duplicate of [GuestBodyTask], which can be used from within this crate
pub(crate) struct GuestBodyTaskInternal<T>(GuestBodyTask<T>);

impl<T, U, Fut> AccessorTask<T, U, wasmtime::Result<()>> for GuestBodyTaskInternal<Fut>
where
    for<'a> U: HasData<Data<'a> = WasiHttpCtxView<'a>>,
    Fut: Future<Output = Result<(), ErrorCode>> + Send + 'static,
{
    async fn run(self, store: &Accessor<T, U>) -> wasmtime::Result<()> {
        self.0
            .run(store, |mut store, trailers| {
                store
                    .get()
                    .table
                    .delete(trailers)
                    .context("failed to delete trailers")
            })
            .await
    }
}

Perhaps this serves as a good data point for the fix

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:50):

alexcrichton created PR review comment:

Ok I pushed an extra commit up with what I was thinking, the rough idea is that the WASI implementations will close over the "getter", the fn(&mut T) -> D::Data<'_> projection, themselves. If that's onerous though to schlep with WASI we can look to bake it in.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:56):

rvolosatovs created PR review comment:

Testing it now, looks like bindgen still needs to be updated for the rename

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:56):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 17:57):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:00):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 25 2025 at 21:33):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Aug 26 2025 at 12:03):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2025 at 12:03):

rvolosatovs created PR review comment:

Unfortunately, I don't think that the refactor fixes the underlying issue, which is that the host interface implementations do not have access to the getter provided by the the call to add_to_linker. The getter provided by Access<T>::getter will always produce &mut T, because the second type parameter defaults to HasSelf<T>.

What this means is that for this to work, the getter would have to be passed to the implementation through WasiSocketsCtxView<'a>, for example.

Accessor::new creates a getter, which is an identity function https://github.com/bytecodealliance/wasmtime/pull/11515/files#diff-714ba43342be98eb093bc17d4ad955a3f3056f4c424635ac399da76349a3a8d7R422, that's the only getter visible to produce implementations

The actual getter implementations need though is only visible to the generated bindings https://github.com/bytecodealliance/wasmtime/pull/11515/files#diff-b365a0c26d559b75058e519f7e01c9e0b201da9f8a16df2b96cf6693f6b38ce6R1458 and it's later used via with_getter to map the 2nd type parameter to a different type https://github.com/bytecodealliance/wasmtime/pull/11515/files#diff-b365a0c26d559b75058e519f7e01c9e0b201da9f8a16df2b96cf6693f6b38ce6R2493-R2520

I'm happy to jump on a quick call to talk through this today to give more details.

Wasmtime GitHub notifications bot (Aug 26 2025 at 12:09):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:31):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:31):

rvolosatovs created PR review comment:

Once a FutureReader is returned by a host implementation, is it guaranteed that the runtime will call close if the guest does not use it? (or e.g. traps)

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:31):

rvolosatovs created PR review comment:

Since we're dealing with byte buffers only here, could we reuse https://docs.rs/tokio/latest/tokio/io/struct.ReadBuf.html directly in some way?

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:31):

rvolosatovs created PR review comment:

I would have expected this function to not be async, in fact, shouldn't this function to move Destination, such that it could only be called once with at most the number of elements that reader asked for?

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:32):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:48):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2025 at 13:48):

rvolosatovs created PR review comment:

nvm, we have figured it out together with @dicej , thanks!

Wasmtime GitHub notifications bot (Aug 26 2025 at 14:08):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2025 at 14:09):

rvolosatovs created PR review comment:

Resolved offline. Once returned to the guest the handle will be properly disposed off with a well-behaving guest. We probably want to introduce some limit on the amount of open future/stream handles that the guest can have

cc @dicej

Wasmtime GitHub notifications bot (Aug 26 2025 at 14:09):

rvolosatovs created PR review comment:

Resolved offline, we may need to wait for realloc, which is why this needs to be async.
@dicej will add a remaining function to this struct

Wasmtime GitHub notifications bot (Aug 26 2025 at 14:09):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2025 at 15:01):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Aug 27 2025 at 13:36):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 27 2025 at 13:46):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 13:46):

rvolosatovs created PR review comment:

I think this is an important detail of the implementation to call out - in the absence of task, the stream handle returned in tuple<stream<T>, future<result<_, E>>> pattern has to be dropped/closed before a value can arrive on the future, I suspect that is because in case of a task, the host could independently track the status of the socket and be notified of the socket shutdown by the OS as part of it's own event loop. Without a task, the guest must effectively "signal" the host to "do work", in success case triggering the Drop of the producer, I suppose.

I wonder if that's the expected behavior here and whether that would play well with GC languages.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:00):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:00):

rvolosatovs created PR review comment:

One solution could be giving both stream and future producers access to the underlying socket and check for shutdown in the future producer as well. It seems that such an approach would require the future producer to also perform reads from the socket and communicate those to stream producer.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:11):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:11):

rvolosatovs created PR review comment:

Giving it a bit more thought, the guest is getting StreamResult::Complete on L46, shouldn't the host eagerly drop the producer before returning this to the guest?

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:11):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:12):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:15):

rvolosatovs deleted PR review comment.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:18):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:18):

dicej created PR review comment:

Giving it a bit more thought, the guest is getting StreamResult::Complete on L46, shouldn't the host eagerly drop the producer _before_ letting the guest know that the stream is done?

Yes, it should; if it's not, I'd consider that a bug. I'll take a look.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:30):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:30):

rvolosatovs created PR review comment:

I've originally mistaken Complete for Dropped, but realized that was wrong https://github.com/bytecodealliance/wit-bindgen/blob/f61634fd7905595ba7b83bb8cda42a4f3ee3ac29/crates/guest-rust/src/rt/async_support/stream_support.rs#L225-L235 and deleted the comment.

I think the test was simply incorrect, see #wasmtime > async API revamp implications

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:33):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:33):

dicej created PR review comment:

I just opened https://github.com/bytecodealliance/wasmtime/issues/11552 to track that.

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:36):

dicej created PR review comment:

Wasmtime doesn't currently depend on tokio except for testing; I'm not sure ReadBuf adds enough value to justify pulling it in.

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:36):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 09:44):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 10:02):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 10:02):

rvolosatovs created PR review comment:

It seems that the consumer MUST loop here to ensure that all of the data already read from the guest has been written - otherwise, if the consumer were to buffer, how would the guest know that a partial write occurred?

It seems that what we really want is to have a way for the consumer to report back the amount of elements actually read, replicating something like https://doc.rust-lang.org/nightly/std/io/trait.Read.html#tymethod.read

Given the existing API, it looks like the consumer should be able to return the buffer, just like the Destination::write would currently do.

This is, of course, already addressed for byte-buffers originating from the guest, but this seems problematic in general case

Wasmtime GitHub notifications bot (Aug 28 2025 at 10:06):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Aug 28 2025 at 11:49):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:18):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:23):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:28):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:31):

rvolosatovs commented on PR #11515:

All existing WASI tests pass now for me locally. I'm a bit surprised that it looks like CI does not run on this PR anymore?

I've removed the reuseaddr test workaround, since this change set should remove the need for it, and close https://github.com/bytecodealliance/wasmtime/issues/11342

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:31):

rvolosatovs edited a comment on PR #11515:

All existing WASI tests pass now for me locally. I'm a bit surprised that it looks like CI does not run on this PR anymore?

I've removed the reuseaddr test workaround, since this change set should remove the need for it and close https://github.com/bytecodealliance/wasmtime/issues/11342

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:35):

rvolosatovs commented on PR #11515:

The main remaining item left here is the cancellation-safety of produce functions, but we've been having some private conversations with @dicej and hopefully a pretty minor API change should suffice to address it.
@dicej please let me know when the new API is available and I will happily fix WASI again.

I will meanwhile work on migrating #11440 to this API

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:36):

rvolosatovs edited a comment on PR #11515:

The main remaining item left here is the cancellation-safety of produce functions, but we've been having some private conversations with @dicej and hopefully a pretty minor API change should suffice to address it.
@dicej please let me know when the new API is available and I will happily fix WASI again.
Could you also rebase this PR on latest main, please?

I will meanwhile work on migrating #11440 to this API

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:39):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:39):

rvolosatovs created PR review comment:

Is "lying" like this OK, @dicej, or do we have to drive I/O and buffer the result?

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:40):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:40):

rvolosatovs created PR review comment:

Same as above, do we need to drive I/O?

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:41):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:41):

rvolosatovs created PR review comment:

same here

Wasmtime GitHub notifications bot (Aug 28 2025 at 12:42):

rvolosatovs commented on PR #11515:

On a second thought, I'm guessing that "lying" about the stream being open is probably not the expected behavior, so I'll work on driving I/O in these and buffering first

Wasmtime GitHub notifications bot (Aug 28 2025 at 13:44):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 13:59):

dicej commented on PR #11515:

On a second thought, I'm guessing that "lying" about the stream being open is probably not the expected behavior, so I'll work on driving I/O in these and buffering first

This is a good reminder that we should add tests that involve zero-length reads and writes from the guest, asserting that the following non-zero-length read or write completes immediately. So far, this hasn't really been relevant, but once we add p3 support to wasi-libc we'll have real-world code that relies on it.

Wasmtime GitHub notifications bot (Aug 28 2025 at 14:26):

rvolosatovs commented on PR #11515:

On a second thought, I'm guessing that "lying" about the stream being open is probably not the expected behavior, so I'll work on driving I/O in these and buffering first

This is a good reminder that we should add tests that involve zero-length reads and writes from the guest, asserting that the following non-zero-length read or write completes immediately. So far, this hasn't really been relevant, but once we add p3 support to wasi-libc we'll have real-world code that relies on it.

I was under impression that we wanted to avoid buffering, however buffering seems to be the only way of providing this behavior for writes to e.g. files. It looks like for filesystem we'll need to introduce a construct (similar to p2), where we'll have a thread performing file I/O and wasi:filesystem write just sends the buffer on something like mpsc channel.

Is that the expectation here?

I think makes sense to me, but it appears we'd also need to introduce sync/flush or similar to wasi:filesystem to be able to ensure that the host worker thread buffer has actually been passed over to the kernel

Wasmtime GitHub notifications bot (Aug 28 2025 at 14:26):

rvolosatovs edited a comment on PR #11515:

On a second thought, I'm guessing that "lying" about the stream being open is probably not the expected behavior, so I'll work on driving I/O in these and buffering first

This is a good reminder that we should add tests that involve zero-length reads and writes from the guest, asserting that the following non-zero-length read or write completes immediately. So far, this hasn't really been relevant, but once we add p3 support to wasi-libc we'll have real-world code that relies on it.

I was under impression that we wanted to avoid buffering, however buffering seems to be the only way of providing this behavior for writes to e.g. files. It looks like for filesystem we'll need to introduce a construct (similar to p2), where we'll have a thread performing file I/O and wasi:filesystem write just sends the buffer on something like mpsc channel.

Is that the expectation here?

I think that makes sense to me, but it appears we'd also need to introduce sync/flush or similar to wasi:filesystem to be able to ensure that the host worker thread buffer has actually been passed over to the kernel

Wasmtime GitHub notifications bot (Aug 28 2025 at 14:27):

rvolosatovs edited a comment on PR #11515:

On a second thought, I'm guessing that "lying" about the stream being open is probably not the expected behavior, so I'll work on driving I/O in these and buffering first

This is a good reminder that we should add tests that involve zero-length reads and writes from the guest, asserting that the following non-zero-length read or write completes immediately. So far, this hasn't really been relevant, but once we add p3 support to wasi-libc we'll have real-world code that relies on it.

I was under impression that we wanted to avoid buffering, however buffering seems to be the only way of providing this behavior for writes to e.g. files. It looks like for filesystem we'll need to introduce a construct (similar to p2), where we'll have a thread performing file I/O and wasi:filesystem write just sends the buffer on something like mpsc channel.

Is that the expectation here?

I think that makes sense to me, but it appears we'd also need to introduce sync/flush or similar to wasi:filesystem to be able to ensure that the host worker thread buffer has actually been passed over to the kernel since stream.write will no longer guarantee that

Wasmtime GitHub notifications bot (Aug 28 2025 at 14:29):

rvolosatovs commented on PR #11515:

hmm, sync does exist https://github.com/WebAssembly/wasi-filesystem/blob/d81d6256c271fe1c8937eb8353e2ddc25517c153/wit-0.3.0-draft/types.wit#L434, so would we make it async then and await the flush before returning?

Wasmtime GitHub notifications bot (Aug 28 2025 at 14:30):

rvolosatovs edited a comment on PR #11515:

hmm, sync does exist https://github.com/WebAssembly/wasi-filesystem/blob/d81d6256c271fe1c8937eb8353e2ddc25517c153/wit-0.3.0-draft/types.wit#L434, so would we need to await the flush before returning?

Wasmtime GitHub notifications bot (Aug 28 2025 at 15:11):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2025 at 15:11):

dicej created PR review comment:

Yeah, I can see how that could be awkward -- you read items from the Source and then try to write them to some kind of sink, but that sink might not be immediately ready to accept all the items. In that case do you just .await until the sink has accepted them all? If you do, then you're kind of blocking the original writer more than you should -- you'd rather just say you didn't read all the items and let the writer write them again if and when it wants to. Also, what if the sink closes before you can write them all?

So yeah, I think we might need to change this API as well as the StreamProducer one.

Wasmtime GitHub notifications bot (Aug 28 2025 at 15:46):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Aug 28 2025 at 16:04):

rvolosatovs commented on PR #11515:

for reference: #wasi > 0-length stream writes with files and stdio

Wasmtime GitHub notifications bot (Sep 01 2025 at 11:28):

sunfishcode commented on PR #11515:

hmm, sync does exist https://github.com/WebAssembly/wasi-filesystem/blob/d81d6256c271fe1c8937eb8353e2ddc25517c153/wit-0.3.0-draft/types.wit#L434, so would we need to await the flush before returning?

There are two kinds of synchronizing here: one is that the data will be visible to independent readers, and the other is that the data will be visible to independent readers even after an abrupt power failure. wasil-filesystem's sync is for that second kind, which can be very slow, so we shouldn't use it unless the application has explicitly requested it.

Wasmtime GitHub notifications bot (Sep 02 2025 at 21:57):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Sep 02 2025 at 22:09):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Sep 03 2025 at 16:51):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 03 2025 at 22:12):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Sep 03 2025 at 23:28):

dicej updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 09:48):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:35):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:35):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:44):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:45):

rvolosatovs created PR review comment:

this would always return None, wouldn't it? (since that's a guest buffer)

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:45):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 10:45):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Sep 04 2025 at 13:58):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 13:58):

dicej created PR review comment:

Good point; I didn't revisit that code after I added the if/else block around it.

Wasmtime GitHub notifications bot (Sep 04 2025 at 17:58):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 18:00):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 18:03):

rvolosatovs updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 18:06):

rvolosatovs commented on PR #11515:

I'm signing off for today and have just finished the WASI crate adaptation a few minutes ago. I have not even really proof-read, for example, the read_directory implementation yet.
There's quite a bit of room for improvement, mainly refactoring and cutting down on duplication.
I also noticed that read_directory seems to not be tested at all.

That all said, all tests pass (at least locally, will have to wait for CI) and IMO this is "good enough" to get this PR merged. Feel free to clean-up as desired, otherwise I will do that tomorrow.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton commented on PR #11515:

I talked with @dicej about this and we concluded that let's go ahead and land this. I've got follow-up feedback and Joel's got some follow-up implementation work but we feel it's best to land this and iterate rather than continuing to block this. The hope is that by landing this @rvolosatovs you're more-or-less unblocke to continue to work on wasi-http while we continue to smith some details here in parallel

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton updated PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton has marked PR #11515 as ready for review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton requested alexcrichton for a review on PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton requested wasmtime-core-reviewers for a review on PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton requested wasmtime-wasi-reviewers for a review on PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton created PR review comment:

Can we skip this fallback and rely on as_direct_destination always succeeding?

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton created PR review comment:

I opened https://github.com/WebAssembly/wasi-cli/issues/81 for this, but for now it might be good to log::warn! the error too

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton created PR review comment:

How come this needs a special case vs the below case?

Wasmtime GitHub notifications bot (Sep 04 2025 at 20:58):

alexcrichton has enabled auto merge for PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

For stdio in particular I think we'll additionally want to call poll_flush here before returning Completed because otherwise tokio only puts this off on some other task and it hasn't actually made its way to stdout yet.

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

I'm going to look into restructuring this function to hopefully not need duplication in this match here and the one far below after the spawn, but that's just refactoring a bit.

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

Like writes below I think this'll want a special-case of 0-length buffers to do a poll_read_ready here

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

This prompted discussion between Joel and I and led to https://github.com/WebAssembly/component-model/issues/561 but we'll need to carefully handle 0-length reads here

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

Talked with Joel about this and he's going to refactor things so the fallback isn't necessary (e.g. as_direct_destination always returns non-None)

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:30):

alexcrichton created PR review comment:

(talked with Joel and I'll handle this in a follow-up)

Wasmtime GitHub notifications bot (Sep 04 2025 at 21:35):

alexcrichton merged PR #11515.

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:34):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:34):

rvolosatovs created PR review comment:

With a 0-length read this tries a buffered read and if that would block, it polls readiness.
From what I understand that's the expectation of this API - produce at least 1 (buffered) element.

Is there a reason to poll readiness before optimistically trying to read?

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2025 at 22:59):

alexcrichton created PR review comment:

True! I should clarify my comment more... I'm assuming that with @dicej's queued up changes the None case for as_direct_destination goes away and so I was assuming that all the non-as_direct_destination code was going to be deleted. In that world the 0-length case is missed here, but otherwise you're right the 0-length case is handled through the buffers otherwise. (although that has its own problems)

Last updated: Dec 06 2025 at 06:05 UTC