Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async/stream/future support for wasmtime-wit-bindgen #10044

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Jan 17, 2025

I've split this out of #9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding generator, along with placeholder type and function definitions in the wasmtime crate which the generated bindings can refer to. See https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates for the design and rationale.

Note that I've added temporary [patch.crates-io] overrides in Cargo.toml until bytecodealliance/wit-bindgen#1130 and bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a T: 'static bound for AsContextMut<Data = T> when generating bindings with concurrent_imports: true. This is only because rustc insists that the closure we're passing to
LinkerInstance::func_wrap_concurrent captures the lifetime of T despite my best efforts to convince it otherwise. Alex and I suspect this is a limitation in the compiler, and I asked about it on the rust-lang Zulip, but we haven't been able to determine a workaround so far.

@dicej dicej requested a review from alexcrichton January 17, 2025 22:50
@rvolosatovs
Copy link
Member

rvolosatovs commented Jan 21, 2025

Also note that we emit a T: 'static bound for AsContextMut<Data = T> when generating bindings with concurrent_imports: true. This is only because rustc insists that the closure we're passing to
LinkerInstance::func_wrap_concurrent captures the lifetime of T despite my best efforts to convince it otherwise. Alex and I suspect this is a limitation in the compiler, and I asked about it on the rust-lang Zulip, but we haven't been able to determine a workaround so far.

In case it helps:

Since async is involved, any chance the error looks anything like rust-lang/rust#63033 ?
If so, a pretty simple fix I have found looks as follows:

That's the same workaround used in rustc internally: https://github.com/rust-lang/rust/blob/ebbe63891f1fae21734cb97f2f863b08b1d44bf8/compiler/rustc_data_structures/src/captures.rs

Another async bug I've encountered, which appears when Send bound is involved is rust-lang/rust#96865, it sometimes manifests as lifetime issues, but looks like the following when minimized: https://docs.rs/send-future/latest/send_future/trait.SendFuture.html
The fix is straightforward: https://github.com/rvolosatovs/send-future/blob/5637a5982b16b1938680ae9ecac659bdad28b5a7/src/lib.rs#L53-L60

@dicej
Copy link
Contributor Author

dicej commented Jan 21, 2025

Thanks for the links, @rvolosatovs. The compiler error I'm seeing doesn't match the ones in those issues, so I suspect it's something different. Here's a (somewhat) minimal example of what I'm seeing:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cd04abfec0133f3a4c0ed7ab53b04d4b

crates/component-macro/src/bindgen.rs Show resolved Hide resolved
@@ -62,6 +62,7 @@ semver = { workspace = true, optional = true }
smallvec = { workspace = true, optional = true }
hashbrown = { workspace = true, features = ["default-hasher"] }
bitflags = { workspace = true }
futures = { workspace = true, features = ["alloc"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be made an optional dependency activated by component-model-async?

I've split this out of bytecodealliance#9582 to make review easier.

This patch adds async/stream/future/error-context support to the host binding
generator, along with placeholder type and function definitions in the
`wasmtime` crate which the generated bindings can refer to.  See
https://github.com/dicej/rfcs/blob/component-async/accepted/component-model-async.md#componentbindgen-updates
for the design and rationale.

Note that I've added temporary `[patch.crates-io]` overrides in Cargo.toml until
bytecodealliance/wit-bindgen#1130 and
bytecodealliance/wasm-tools#1978 have been released.

Also note that we emit a `T: 'static` bound for `AsContextMut<Data = T>` when
generating bindings with `concurrent_imports: true`.  This is only because
`rustc` insists that the closure we're passing to
`LinkerInstance::func_wrap_concurrent` captures the lifetime of `T` despite my
best efforts to convince it otherwise.  Alex and I suspect this is a limitation
in the compiler, and I asked about it on the rust-lang Zulip, but we haven't
been able to determine a workaround so far.

Signed-off-by: Joel Dice <[email protected]>

remove obsolete TODO comment

Signed-off-by: Joel Dice <[email protected]>

make `futures` dep optional

Signed-off-by: Joel Dice <[email protected]>

update `wasm-tools` and `wit-bindgen`

Signed-off-by: Joel Dice <[email protected]>
@dicej dicej force-pushed the wasmtime-wit-bindgen-async branch from 24365da to f04b932 Compare January 22, 2025 15:57
@dicej dicej marked this pull request as ready for review January 22, 2025 16:52
@dicej dicej requested review from a team as code owners January 22, 2025 16:52
@dicej dicej requested review from pchickey and removed request for a team January 22, 2025 16:52
@dicej dicej added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
Signed-off-by: Joel Dice <[email protected]>
@dicej dicej enabled auto-merge January 22, 2025 17:04
@dicej dicej added this pull request to the merge queue Jan 22, 2025
Merged via the queue into bytecodealliance:main with commit 636435f Jan 22, 2025
39 checks passed
@dicej dicej deleted the wasmtime-wit-bindgen-async branch January 22, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants