Stream: git-wasmtime

Topic: wasmtime / PR #11325 refactor `{Stream,Future}|{Reader,Wr...

Wasmtime GitHub notifications bot (Jul 25 2025 at 21:38):

dicej requested alexcrichton for a review on PR #11325.

Wasmtime GitHub notifications bot (Jul 25 2025 at 21:38):

dicej opened PR #11325 from dicej:stream-future-refactor-upstream to bytecodealliance:main:

This makes a several changes to how {Stream,Future}|{Reader,Writer} work to make them more efficient and, in some ways, more ergonomic:

• The background tasks have been removed, allowing reads and writes to complete without task context switching. We now only allocate and use oneshot channels lazily when the other end is not yet ready; this improves real world performance benchmarks (e.g. wasi-http request handling) considerably.

• Instances of {Stream,Future}Reader can now be lifted and lowered directly; no need for Host{Stream,Future} anymore.

• The type parameter for Stream{Reader,Writer} no longer refers to the buffer type -- just the payload type (i.e. StreamReader<u8> instead of StreamReader<Vec<u8>>), meaning any buffer type may be used for a given read or write operation. This also means the compiler needs help with type inference less often when calling Instance::stream.

• Instances of {Stream,Future}|{Reader,Writer} now require access to the store in order to be disposed of properly. I've added RAII wrapper structs (WithAccessor[AndValue]) to help with this, and also updated Store::drop and Instance::run_concurrent to ensure the store thread-local is set when dropping futures closing over &Accessors.

• In order to ensure that resources containing {Stream,Future}|{Reader,Writer} instances are disposed of properly, I've added LinkerInstance::resource_concurrent and have updated wasmtime-wit-bindgen to use it. This gives resource drop functions access to a StoreContextMut via an Accessor, allowing the stream and future handles to be disposed of.
  - In order to make this work, I had to change Accessor::instance from a Instance to an Option<Instance>, which is awkward but temporary since we're planning to remove Accessor::instance entirely once we've moved concurrent state from ComponentInstance to Store.

That problem of disposal is definitely the most awkward part of all this. In simple cases, it's easy enough to ensure that read and write handles are disposed of properly, but both wasmtime-wasi and wasmtime-wasi-http have some pretty complicated functions where handles are passed between tasks and/or stored inside resources, so it can be tricky to ensure proper disposal on all code paths. I'm open to ideas for improving this, but I suspect we'll need new Rust language features (e.g. linear types) to make it truly ergonomic, robust, and efficient.

While testing the above, I discovered an issue with Instance::poll_until such that it would prematurely give up and return a "deadlock" trap error, believing that there was no further work to do, even though the future passed to it was ready to resolve the next time it was polled. I've fixed this by polling it one last time and only trapping if it returns pending.

Note that I've moved a few associated functions from ConcurrentState to Instance (e.g. guest_drop_writable and others) since they now need access to the store; they're unchanged otherwise. Apologies for the diff noise.

Finally, I've tweaked how wasmtime serve to poll the guest for content before handing the response to Hyper, which helps performance by ensuring the first content chunk can be sent with the same TCP packet as the beginning of the response.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jul 25 2025 at 21:38):

dicej requested wasmtime-wasi-reviewers for a review on PR #11325.

Wasmtime GitHub notifications bot (Jul 25 2025 at 21:38):

dicej requested wasmtime-core-reviewers for a review on PR #11325.

Wasmtime GitHub notifications bot (Jul 25 2025 at 22:14):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

I believe this isn't sound due to the self.value.take() in the destructor, notably that moves the future meaning self-poitners are all invalidated, meaning that the drop could go wrong.

Destructors and Pin are quite subtle which would make me want to trend more towards a safer version of this rather than having unsafe here. The pin_project_lite! macro has support for Drop but unfortunately I'm not sure it would help in this case because that'd just give you Pin<&mut V> in the destructor which isn't sufficient for actual destruction.

I know that at least one way to model this is to use ManuallyDrop to wrap the field insted of Option, meaning that the Drop for Dropper would unsafely drop-in-place inside of tls::set. That's still unsafe code, however, either with or without pin_project!. To me though that's probably the best we can do for now, so how about:

• Add pin_project_lite as a dependency of Wasmtime component-model-async and use it here.
  • Unrelated to this PR we should probably use it for vm::{Instance, ComponentInstance} to remove some unsafe code there.
• Use ManuallyDrop to store value instead of Option
• Use unsafe code to, in the destructor, run drop-in-place on Pin<&mut ManuallyDrop<V>> by unsafely getting access to &mut ManuallyDrop<V>.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

WDYT about changing this to &mut self? I feel like that's a bit more idiomatic and maps well to Drop?

Also I think this trait will want to be clearly documented in its relation to Drop. Specifically this trait implementation does nothing outside of WithAccessor and that's required to do anything here.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

Similar to the above, as an async-cancel point this'll need some sort of destructor to run the self.drop(store) below.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

Could this be surfaced as a pub fn close(...) on StreamWriter itself? (and the reader, possibly futures too)

Basically I think it'd be good to document this irrespective of the DropWithStore impl

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

Something here will need to change, but I'm not exactly sure how and what. I think this is sound but it's panick-y because host_write can call accept_reader which can lower a value into wasm which isn't valid to do unless we're on a fiber. That runs the risk of suspending Rust code when there's embedder frames on the stack, which ok that's unsound and not just panick-y.

One option is to drop this all entirely and have documentation indicating that a future cannot be closed without sending a value. That's not fantastic, however, as cancellation in Rust has no means of propagating into wasm (e.g. sending a None or something like that).

Another alternative would be to change host_write to not execute the lowering immediately but to instead kick of a wasm task in the background of sorts to perform the lowering. Unsure.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

This is a bit tricky because this method takes self-by-value which means there's no With* on the stack that can run the destructor, and as an async cancel point here dropping this future wouldn't actually clean up the future itself.

This sort of relates to the DropWithStoreAndValue comment I have below. On one hand this is a fundamental mismatch with "channel semantics" and "that's a future" where things are coming to a clash trying to model WIT futures in Rust in the face of cancellation. On the other hand this is solvable via similar mechanisms to below by retaining the default-value-to-send closure and arranging that to be sent at some point during destruction.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

In the abstract I think I personally prefer to keep the closure-to-manufacture-the-value in the constructor of a future rather than having this trait. Would that still work?

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:04):

alexcrichton created PR review comment:

Bikeshedding this a bit, this feels like it should be called DropWithAccessor or somehow have "drop" related to the name since "with accessor" is a super generic name that doesn't quite imply what's happening under the hood here. Not that I like the name DropWithAccessor though, that also feels bad...

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:07):

alexcrichton commented on PR #11325:

This is going to have conflicts with https://github.com/bytecodealliance/wasmtime/pull/11328, and I'll help work through those.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:21):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2025 at 18:21):

dicej created PR review comment:

Good point. Yeah, I think we might need a background task here; I forgot this is one of the reasons I was doing _everything_ on a background task until this PR.

Wasmtime GitHub notifications bot (Jul 28 2025 at 22:17):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 28 2025 at 22:19):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2025 at 22:19):

dicej created PR review comment:

I've switched to ManuallyDrop. It turned out pin-project-lite didn't buy us much, since we needed to unsafely convert from Pin<&mut ManuallyDrop<F>> to Pin<&mut F> anyway, so I didn't end up using it.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:50):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:51):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:51):

dicej created PR review comment:

I've updated this code to use WithAccessor and eliminate DropWithStoreAndValue.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:52):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:52):

dicej created PR review comment:

This is no longer part of the public API, so we can change the name if/when we have a better one, or just not worry about it.

Wasmtime GitHub notifications bot (Jul 29 2025 at 21:54):

dicej commented on PR #11325:

I've addressed all the feedback so far except that we're still lowering writes using same stack the host embedder called us on; I'll address that by using a fiber for lowering.

Wasmtime GitHub notifications bot (Jul 29 2025 at 22:09):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 29 2025 at 23:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2025 at 23:33):

alexcrichton created PR review comment:

Think about this in the context of rebasing with wasi:sockets -- is this necessary? Tasks can only be cancelled for two reasons:

1. The store is dropped - in this case there's no need to do further cleanup
2. The AbortHandle is triggered, but in this case it's statically known we just forget about that and ignore it.

Given that does WASI actually need Guarded*?

Wasmtime GitHub notifications bot (Jul 29 2025 at 23:42):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2025 at 23:42):

dicej created PR review comment:

Perhaps we don't _need_ it, but it saves us from having to remember to close the handle explicitly before returning, which isn't too onerous for this simple task, but could become a maintenance hazard later.

wasmtime-wasi-http has some pretty complicated control flow, where keeping track of what to close when without RAII would be a real pain.

Wasmtime GitHub notifications bot (Jul 30 2025 at 14:16):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 30 2025 at 14:18):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2025 at 14:18):

dicej created PR review comment:

I just pushed an update to ensure that all host writes use a background task to lower when there may be embedder frames on the stack.

Wasmtime GitHub notifications bot (Jul 30 2025 at 15:18):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 30 2025 at 15:43):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 30 2025 at 15:47):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 30 2025 at 15:49):

dicej updated PR #11325.

Wasmtime GitHub notifications bot (Jul 30 2025 at 15:54):

dicej commented on PR #11325:

@alexcrichton I believe I've addressed all your feedback so far. Let me know if you have anything else.

BTW, the optimization for lowering flat payloads during host writes is pretty trivial:

diff --git a/crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs b/crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs
index 2809cb6b12..ed56fd7bce 100644
--- a/crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs
+++ b/crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs
@@ -1777,9 +1777,7 @@ impl Instance {
                     anyhow::Ok(result)
                 };

-                if
-                // TODO: Check if payload is "flat"
-                false {
+                if T::IS_FLAT_TYPE {
                     // Optimize flat payloads (i.e. those which do not require
                     // calling the guest's realloc function) by lowering
                     // directly instead of using a oneshot::channel and
diff --git a/crates/wasmtime/src/runtime/component/func/typed.rs b/crates/wasmtime/src/runtime/component/func/typed.rs
index ad4328fce4..f27120957a 100644
--- a/crates/wasmtime/src/runtime/component/func/typed.rs
+++ b/crates/wasmtime/src/runtime/component/func/typed.rs
@@ -687,6 +687,9 @@ pub unsafe trait ComponentType: Send + Sync {
     #[doc(hidden)]
     const IS_RUST_UNIT_TYPE: bool = false;

+    #[doc(hidden)]
+    const IS_FLAT_TYPE: bool = false;
+
     /// Returns the number of core wasm abi values will be used to represent
     /// this type in its lowered form.
     ///
@@ -1016,6 +1019,8 @@ macro_rules! integers {

             const ABI: CanonicalAbiInfo = CanonicalAbiInfo::$abi;

+            const IS_FLAT_TYPE: bool = true;
+
             fn typecheck(ty: &InterfaceType, _types: &InstanceType<'_>) -> Result<()> {
                 match ty {
                     InterfaceType::$ty => Ok(()),

Wasmtime GitHub notifications bot (Jul 30 2025 at 16:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2025 at 17:07):

alexcrichton merged PR #11325.

Last updated: Dec 06 2025 at 06:05 UTC