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

ENH make all Tauri commands async #218

Merged
merged 5 commits into from
Dec 14, 2024
Merged

ENH make all Tauri commands async #218

merged 5 commits into from
Dec 14, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Aug 29, 2024

See Tauri documentation on async commands. Synchronous commands will run on the main thread, which, noticeable or not, blocks UI rendering and other things that run on the main thread. See also the YouTube video made by one of the Tauri maintainers.

The things is, there is no reason to use synchronous commands. We currently call them only from the frontend, which is already an asynchronous behavior. By running on a separate thread we avoid freezing the UI and the theoretical performance would always be better. Though we may not notice currently, when developing #66 I made a very long-running command bundle_external, and if not made async, it makes the manager window completely unresponsive during its execution.

This is a major change so ping @Xinyu-Li-123 if you are interested.

Copy link

github-actions bot commented Aug 29, 2024

✔️ Deskulpt Built Successfully!

Deskulpt binaries have been built successfully on all supported platforms. Your pull request is in excellent shape! You may check the built Deskulpt binaries here and download them to test locally.

Workflow file: .github/workflows/build.yaml. Generated for commit: 113a4c2.

@@ -7,6 +7,7 @@ edition = "2021"
tauri-build = { version = "2.0.0-beta.13", features = ["codegen"] }

[dev-dependencies]
async-std = { version = "1.12.0", features = ["attributes"] }
Copy link
Contributor Author

@Charlie-XIAO Charlie-XIAO Aug 29, 2024

Choose a reason for hiding this comment

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

@Xinyu-Li-123
Copy link
Contributor

I agree that we should make the corresponding rust apis async. But I do have a question. Are you sure synchronous tauri command will block the UI rendering? According to the Tauri process model, the (one and only) core process is responsible for executing the command, while the webview processes are responsible for rendering the UI.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Aug 31, 2024

That's interesting. Quote Tauri docs:

Asynchronous functions are benefical in Tauri to perform heavy work in a manner that doesn't result in UI freezes or slowdowns.

So I think UI rendering will indeed suffer from long-running synchronous commands. I'm thinking that though the core process is not in charge of the actual rendering, it still needs to do something to trigger the rendering in the Webview process and perhaps it's this thing that gets blocked. I mean, the Webview processes may be able to do only limited things, and those that require collaboration/communication with the core process will still freeze. Wild guess though 🤔

@Xinyu-Li-123
Copy link
Contributor

Xinyu-Li-123 commented Sep 2, 2024

I saw that you made all internal commands async, but you leave the widget apis sync. I wonder if this is a design choice or is it just unfinished work?

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Sep 2, 2024

Thanks for noticing that - it's my oversight and I'll fix soon :)

@Xinyu-Li-123
Copy link
Contributor

Xinyu-Li-123 commented Sep 2, 2024

Could you elaborate on how the manager window freezes in #66? The frontend command for widget bundling is already async, so the manager window shouldn't freeze unless there is extensive resource usage, in which case it's cpu-intensive rather than io-intensive and concurrency shouldn't be helpful.

@Xinyu-Li-123
Copy link
Contributor

Xinyu-Li-123 commented Sep 2, 2024

Also, I'd like to point out that, although less efficient, synchronous Rust apis makes programming easier on Rust side. For example, if we make the fs api asynchronous, we will need to consider concurrent access to the same file. Even each widget can only access its own resource, this could still happen if one widget invokes multiple apis in a short period, such as quickly creating multiple todolist items. I'm not sure if our use case could really gain benefit from asynchronous Rust apis, given our frontend apis are already asynchronous.

@Charlie-XIAO
Copy link
Contributor Author

Also, I'd like to point out that, although less efficient, synchronous Rust apis makes programming easier on Rust side. For example, if we make the fs api asynchronous, we will need to consider concurrent access to the same file, which could happen if one widget invokes multiple apis in a short period. I'm not sure if our use case could really gain benefit from asynchronous Rust apis, given our frontend apis are already asynchronous.

I assume you are speaking of cases where we want to call the commands internally in Rust code. In widget APIs our endpoints will finally all be async (as they are meant to be called from the frontend); in this case we just need to add .await and that's not complicated. In the core things could become trickier, but we have the tauri::async_runtime::block_on as the last resolution. Another way is to make a separate sync function and wrap it into an async command.

Could you elaborate on how the manager window freezes in #66? The frontend command for widget bundling is already async, so the manager window shouldn't freeze unless there is extensive resource usage, in which case it's cpu-intensive rather than io-intensive and concurrency shouldn't be helpful.

I will try to make a minimal repo to see if I can showcase the problem. Hopefully that would provide more insight.

@Charlie-XIAO
Copy link
Contributor Author

@Xinyu-Li-123
Copy link
Contributor

Xinyu-Li-123 commented Sep 19, 2024

Sorry for the delay. I've been busy with some personal stuff.

Thanks for the demo tauri app. It seems like blocking in core process will somehow freeze the webview process. Building upon your example, I have further tried the following setups

  • running tauri dev with two webview windows, both freezes when running sync greet
  • running tauri build with two webview windows, same things happen

As a result, I agree that it's reasonable to make all Tauri commands async, both commands for manager and those for widget. For next step about this PR, I'd like to further review if there is any concurrency issue. I suspect that simply adding async keyword to all widget-related tauri commands won't work, even if the resources of a widget is only accessed by itself.

If no such bug is found, I will explicitly confirm it in the comment.

@Xinyu-Li-123
Copy link
Contributor

I think this PR is ready to be merge.

@Xinyu-Li-123 Xinyu-Li-123 merged commit 5336dde into main Dec 14, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants