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

feat: Add features for async runtimes on Linux #248

Merged
merged 2 commits into from
May 21, 2023

Conversation

DataTriny
Copy link
Member

@DataTriny DataTriny commented May 1, 2023

Fixes #227

This PR adds new crate features to accesskit_unix and accesskit_winit. They are designed to specify which async runtime to use for D-Bus communication:

  • async-io: rely on zbus's support for async-std,
  • tokio: use a dedicated tokio runtime.

The default is async-io, which keeps the current behavior unchanged.

This is indeed suboptimal if zbus is used somewhere else as we won't use a common runtime in the case of tokio.

@DataTriny
Copy link
Member Author

@lunixbochs Could you maybe give this a try? Our winit example works just fine but your setup might differ.

Thanks very much.

@lunixbochs
Copy link
Contributor

thanks, I'll try to test this later today.

looking at the changes required here - instead of a private runtime I'd ultimately rather have a public async api in accesskit, or to file/fix the upstream bug in zbus, but this may be a good stopgap.

@lunixbochs
Copy link
Contributor

lunixbochs commented May 9, 2023

yep, this fixes the crash for me. I run into other problems now that it works, but I don't think they're related to zbus or this PR.

@lunixbochs
Copy link
Contributor

oh I see the other problem I had was the app not showing up in accerciser, but I just saw #241 which explains it.

I was able to open the at-spi bus manually with d-feet and poke around my app's UI hierarchy.

@DataTriny
Copy link
Member Author

Thanks very much @lunixbochs for your feedback on this.

I'm still open to the idea of exposing an async API (the underlying code is pretty much all async anyway now), but would it really fit nicely into most projects? This would be the only adapter to expose such an API, therefore it would make writing cross-platform code more complex I guess.

I don't have much expertise when it comes to async Rust, but I honestly don't know how zbus could be improved here. zbus is runtime agnostic, it's just that tokio seems to expose a different API.

@lunixbochs
Copy link
Contributor

This would be the only adapter to expose such an API

I think you'd be able to bridge in both directions, so you can use either the sync or async api on all platforms, and let the caller know the platform-specific tradeoffs.

I don't have much expertise when it comes to async Rust, but I honestly don't know how zbus could be improved here. zbus is runtime agnostic, it's just that tokio seems to expose a different API.

I'm going to respond to this on the linked issue rather than this PR.

@mwcampbell
Copy link
Contributor

Why is tokio now an unconditional dev dependency of accesskit_winit? Is this a leftover from testing?

Also, I'd like two other changes to this PR:

  1. In platforms/unix/Cargo.toml, I think the tokio feature should explicitly enable zbus/tokio, as the async-io feature does for zbus/async-io, rather than assuming that some other crate in the graph will enable zbus/tokio.
  2. Let's use once_cell instead of lazy_static, for consistency with what I've done elsewhere in AccessKit.

@mwcampbell mwcampbell merged commit b56b4ea into AccessKit:main May 21, 2023
@mwcampbell mwcampbell mentioned this pull request May 21, 2023
@DataTriny DataTriny deleted the fix_zbus_executor branch June 24, 2023 15:31
lunixbochs pushed a commit to talonvoice/accesskit that referenced this pull request Aug 31, 2023
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.

zbus panic on Linux
3 participants