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

fix: Run our own async executor on Unix #337

Merged
merged 4 commits into from
Jan 10, 2024
Merged

fix: Run our own async executor on Unix #337

merged 4 commits into from
Jan 10, 2024

Conversation

DataTriny
Copy link
Member

Fixes #332

Depends on #336

Unix adapters can now be used inside both a sync and an async context. All D-Bus communications now happen on another thread, which runs its own async executor. The internal executor of zbus connections are disabled and replaced by tasks which run on the same thread. Abstraction helpers have been copied from zbus to be as much runtime agnostic as possible.

This PR can be tested by modifying the accesskit_winit simple example:

  • Add either async-std or tokio as a dev dependency,
  • Make the main function async,
  • Annotate it with either #[async_std::main] or #[tokio::main],
  • Run the example with default features or with the tokio one,
  • Make sure the program doesn't panic or hang.

@ids1024
Copy link

ids1024 commented Jan 8, 2024

With this change (unlike the current main branch), the winit simple example panics if zbus = { version = "3", features = ["tokio"] } is added as a dependency:

thread '<unnamed>' panicked at /home/ian/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.32.0/src/net/unix/stream.rs:849:18:
there is no reactor running, must be called from the context of a Tokio 1.x runtime
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Spawning zbus futures on a non-tokio executor fails if the zbus/tokio feature is enabled. Using the tokio feature of accesskit_unix fixes this. But the top-level crate or another dependency adding a feature flag to zbus shouldn't break accesskit.

I raised this issue on zbus earlier in dbus2/zbus#526. I'm not sure exactly what is the best way for zbus or accesskit to avoid this, but it definitely doesn't seem ideal.

@DataTriny
Copy link
Member Author

@ids1024 Yes we all know the situation isn't ideal here. Of course if you already depend on tokio then you are supposed to activate accesskit_unix's tokio feature (or the accesskit_winit). I think I will add a note about this in the README.

@DataTriny
Copy link
Member Author

We could have also unconditionally used async-io executor even if tokio is used at the application level, but then we force all these async-io dependencies on tokio users. At the moment I decided not to do that and make sure to exclude as much of these crates as possible when the tokio feature is enabled.

@DataTriny
Copy link
Member Author

Maybe I should note that the only reason it doesn't crash currently is because we fallback to zbus::block_on when the tokio feature isn't enabled. Therefore our futures end up on the runtime spawned by zbus. This is something we have to change though because this function isn't really part of zbus API.

@mwcampbell
Copy link
Contributor

Do we still need to use async_once_cell::Lazy to lazily initialize the adapter? The initialization function doesn't use await anywhere, so we could use the simpler, synchronous once_cell::sync::Lazy instead, and eliminate our dependency on async-once-cell.

@mwcampbell
Copy link
Contributor

Just to be safe, can we also issue a compile error if both the async-io and tokio features are set? And is there any way we can check at compile time whether the feature selection for zbus matches the one for our own crate?

@DataTriny
Copy link
Member Author

Do we still need to use async_once_cell::Lazy to lazily initialize the adapter?

I missed that, good catch!

And is there any way we can check at compile time whether the feature selection for zbus matches the one for our own crate?

Unfortunately this is not possible as Cargo doesn't pass dependency features to the crate being compiled. The assumption here is if someone enables the tokio feature of zbus then they know what they are doing thus they must check that all their dependencies are properly set. This is far from ideal but I don't think we can do better right now.

@mwcampbell
Copy link
Contributor

@DataTriny Good work. Are you waiting for anyone else to review this? If not, feel free to merge.

@DataTriny
Copy link
Member Author

I'll just mention that the Unix adapter runs perfectly fine with the default feature inside a tokio-based program, just like zbus.

I'm happy to revisit this if someone comes with a better approach.

@DataTriny DataTriny merged commit 8f937ba into main Jan 10, 2024
5 checks passed
@DataTriny DataTriny deleted the unix-executor branch January 10, 2024 21:45
@mwcampbell mwcampbell mentioned this pull request Jan 8, 2024
@james7132
Copy link

Is there a way to make this customizable? We're trying to keep the threading story for Bevy consistent, and extra threads pose potential performance issues due to the engine's sensitivity to context switches. Requiring async-std or tokio are also both not feasible given our constraints.

@mwcampbell
Copy link
Contributor

@james7132 We have no choice but to depend transitively on async-io or tokio as long as we use zbus. And if we move away from zbus, someone will dislike that we're either using a C library (e.g. via the dbus crate) or fragmenting the ecosystem (by forking zbus). Apparently we can't please everyone at once. This is why we agreed to the compromise of feature-gating the AccessKit Unix adapter in accesskit_winit and in Bevy.

We also have no choice but to start a thread for the D-Bus connection, because winit doesn't give us a way to add our file descriptor(s) to its event loop. accesskit_unix always did that; it was just indirect, via zbus, before. So that's not a regression.

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' blocking API causes panicks for async users
4 participants