Skip to content

refactor: dynamic dispatch async commands #13464

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

refactor: dynamic dispatch async commands #13464

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
07615f0
Dynamic dispatch async commands
Legend-Master May 18, 2025
e2f24f6
format
Legend-Master May 18, 2025
de15f05
Preserve `'static`
Legend-Master May 18, 2025
5f817b9
Use a inner function instead
Legend-Master May 19, 2025
0fb22eb
Only do it for dev for now
Legend-Master May 19, 2025
d4fe467
Add change file
Legend-Master May 19, 2025
549f319
Merge branch 'dev' into dynamic-dispatch-async-commands
Legend-Master May 19, 2025
5a64b09
Tag respond_async_serialized_dyn with debug
Legend-Master May 19, 2025
097c8d5
Merge remote-tracking branch 'upstream/dev' into dynamic-dispatch-asy…
Legend-Master May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions crates/tauri-macros/src/command/wrapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,22 +331,22 @@ fn body_async(
use tracing::Instrument;

let span = tracing::debug_span!("ipc::request::run");
#resolver.respond_async_serialized(async move {
#resolver.respond_async_serialized(Box::pin(async move {
let result = $path(#(#args?),*);
let kind = (&result).async_kind();
kind.future(result).await
}
.instrument(span));
.instrument(span)));
return true;
}

#[cfg(not(feature = "tracing"))]
quote! {
#resolver.respond_async_serialized(async move {
#resolver.respond_async_serialized(Box::pin(async move {
let result = $path(#(#args?),*);
let kind = (&result).async_kind();
kind.future(result).await
});
}));
return true;
}
})
Expand Down
13 changes: 8 additions & 5 deletions crates/tauri/src/ipc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
//!
//! This module includes utilities to send messages to the JS layer of the webview.
use std::sync::{Arc, Mutex};
use std::{
pin::Pin,
sync::{Arc, Mutex},
};

use futures_util::Future;
use http::HeaderMap;
Expand Down Expand Up @@ -338,10 +341,10 @@ impl<R: Runtime> InvokeResolver<R> {
}

/// Reply to the invoke promise with an async task which is already serialized.
pub fn respond_async_serialized<F>(self, task: F)
where
F: Future<Output = Result<InvokeResponseBody, InvokeError>> + Send + 'static,
{
pub fn respond_async_serialized(
self,
task: Pin<Box<dyn Future<Output = Result<InvokeResponseBody, InvokeError>> + Send + 'static>>,
) {
Copy link
Contributor

@WSH032 WSH032 May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Considering that respond_async_serialized is a pub API, isn't this a BREAKING CHANGE?

maybe we could enable this for the debug build and still use the old static dispatch one for the release build?

In my opinion, having different signatures in debug and release modes would be confusing.

Not sure if I am correctly: if we only pass in Box::pin(...) as Pin<Box<dyn ...>>, then only a single template instantiation of respond_async_serialized<Pin<Box<dyn ...>>> will occur, which is effectively the same as changing the signature to Pin<Box<dyn ...>>, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that respond_async_serialized is a pub API, isn't this a BREAKING CHANGE?

I believe this part was marked as unstable?

In my opinion, having different signatures in debug and release modes would be confusing.

That's true though, I feel like the performance hit probably won't be a problem considering this is only a pretty small part of the call chain anyways

Not sure if I am correctly: if we only pass in Box::pin(...) type, then only a single template instantiation of respond_async_serialized will occur, which is effectively the same as changing the signature to Pin<Box<dyn ...>>, right?

I believe the box itself will create a new generic variation for every closure, so this probably won't work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure if this was marked as unstable or not, will double check that tomorrow, but since we manage it through macros, it should be easy to add a new function

Copy link
Member

@FabianLars FabianLars May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it's probably meant to be unstable but we only communicated that on the "parent" Invoke struct.

honestly though? in my opinion marking anything pub in our main tauri crate as unstable only via a doc comment is bullshit. Ideally unstable APIs should not be public, or clearly unstable (feature flag, or in an unstable module path idk) but that's a discussion for later i guess.

Since we're working on v3 i think marking it as deprecated would be the more sensible choice here.

"working" as in praying it will somehow appear out of nowhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I am already directly using Invoke downstream (in pytauri), so I always hope these APIs change as little as possible (v3) 😂

Copy link
Contributor Author

@Legend-Master Legend-Master May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it's probably meant to be unstable but we only communicated that on the "parent" Invoke struct.

Yeah, I think so after checking

honestly though? in my opinion marking anything pub in our main tauri crate as unstable only via a doc comment is bullshit. Ideally unstable APIs should not be public, or clearly unstable (feature flag, or in an unstable module path idk) but that's a discussion for later i guess.

To be fair though, some of the things like generate_handler! and generate_context! and the related code are impossible to be stable and they still have to be public

Normally, users won't use them directly so it's fine to break, but it's probably a problem for the library authors (I pinned the tauri version in tauri-runtime-verso, and also we can see the impact on pytauri as well)

I am already directly using Invoke downstream (in pytauri), so I always hope these APIs change as little as possible (v3) 😂

I have tried to make a inner private function so the public API won't change from this, and it seems to only produce slightly more LLVM IR than doing it from the macro, pushed it

Thanks for the heads up @WSH032 about avoiding this breaking change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the question is now, do we want to use this for both the debug build and the release build, or do we only do this in debug build for now, since this is done with a private inner function, we should be able to change this without any breakages

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested this Box::pin(...) as Pin<Box<dyn ...>>, and from the assembly output it does seem that only one instantiation of respond_async_serialized<Pin<Box<dyn ...>>> occurs: https://godbolt.org/z/9sMMqdo1o

To be fair though, some things like generate_handler! and generate_context! and the related code are impossible to make stable, and they still have to be public

I think I'll use #[doc(hide)] to hide them.

And the question is now, do we want to use this for both the debug build and the release build, or do we only do this in debug build for now

I think everyone has their own preferences. Personally, I don't care about code bloat, so I would prefer using dyn in debug mode to speed up development, and using impl generics in release mode to optimize performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this Box::pin(...) as Pin<Box<dyn ...>>, and from the assembly output it does seem that only one instantiation of respond_async_serialized<Pin<Box<dyn ...>>> occurs: https://godbolt.org/z/9sMMqdo1o

Interesting, we could try that as well (I do want to avoid thing inside the macro in general though, so if the inner function method works, I might prefer that one)

I think I'll use #[doc(hide)] to hide them.

We can't hide the top level macros like generate_handler!, but we probably can hide the other ones, but to be fair, they still show up in IDEs so I'm not sure how effective would this be

I think everyone has their own preferences. Personally, I don't care about code bloat, so I would prefer using dyn in debug mode to speed up development, and using impl generics in release mode to optimize performance.

Good to know, I feel like this probably won't be a problem in terms of performance since the overhead of other things will heavily out weight the cost of this, but I'm fine with keeping it the same in release mode for now

Copy link
Member

Choose a reason for hiding this comment

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

To be fair though, some of the things like generate_handler! and generate_context! and the related code are impossible to be stable and they still have to be public

Well there are exceptions to everything, it just shouldn't be the standard.

crate::async_runtime::spawn(async move {
let response = match task.await {
Ok(ok) => InvokeResponse::Ok(ok),
Expand Down
Loading