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

zbus panic on Linux #227

Closed
lunixbochs opened this issue Mar 14, 2023 · 11 comments · Fixed by #248
Closed

zbus panic on Linux #227

lunixbochs opened this issue Mar 14, 2023 · 11 comments · Fixed by #248

Comments

@lunixbochs
Copy link
Contributor

lunixbochs commented Mar 14, 2023

I get this panic when opening a winit window that creates an AccessKit adapter on Linux.
This is executing in the winit main thread. I called accesskit_winit::Adapter::with_action_handler from my own code.

thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime', /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/zbus-3.7.0/src/abstractions/executor.rs:62:27
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::panic_display
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:72:5
   3: tokio::runtime::scheduler::Handle::current
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/mod.rs:57:27
   4: tokio::runtime::handle::Handle::current
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/handle.rs:102:20
   5: tokio::task::spawn::spawn_inner
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/task/spawn.rs:184:22
   6: tokio::task::spawn::spawn
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/task/spawn.rs:171:13
   7: zbus::abstractions::executor::Executor::spawn
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/zbus-3.7.0/src/abstractions/executor.rs:62:27
   8: accesskit_unix::adapter::Adapter::new
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/accesskit_unix-0.3.2/src/adapter.rs:45:26
   9: accesskit_winit::platform_impl::platform::Adapter::new
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/accesskit_winit-0.12.3/src/platform_impl/unix.rs:21:23
  10: accesskit_winit::Adapter::with_action_handler
             at /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/accesskit_winit-0.12.3/src/lib.rs:170:23
@mwcampbell
Copy link
Contributor

Please provide the output of cargo tree. Make sure to either attach it, or enclose it in a Markdown code block to preserve formatting. I'm asking for this because AccessKit doesn't use zbus's tokio feature; it uses the async-io feature instead. But I wonder if you have zbus with the tokio feature somewhere else in your transitive dependencies.

@lunixbochs
Copy link
Contributor Author

lunixbochs commented Mar 14, 2023

It's not (just) transitive - I use zbus with the tokio feature in my project. Looking at the zbus executor code, it seems extremely fragile in this case? Switching between self.executor and tokio::task::spawn based on a global crate feature is clearly unsound.

@mwcampbell
Copy link
Contributor

Then I suppose accesskit_unix needs to have async-io and tokio features, and work with both runtimes. @DataTriny Can you look into this?

@lunixbochs Just curious, what project are you using this in? If you can't share, no worries.

@lunixbochs
Copy link
Contributor Author

lunixbochs commented Mar 14, 2023

Then I suppose accesskit_unix needs to have async-io and tokio features, and work with both runtimes. @DataTriny Can you look into this?

zbus still feels really fragile here, I'm now worried something else in my dep tree will have the same problem. zbus can only work if everything in your dependency tree uses the same executor, this is clearly a bug in the zbus api/design :(

Just curious, what project are you using this in?

I'm porting the core app/ui in Talon from Qt to Rust (egui) + a cool actor model system I built for UI + a handful of custom crates for stuff like better tray icon/menu and clipboard support to bring things up to par with my Qt codebase. I'm hoping to not regress on screen reader support in the process, as Qt native windows come with accessibility integration for free.

FWIW here are my results so far trying latest accesskit with egui in Talon (I'm on latest winit and I patched egui to use latest accesskit):

  • macOS: an AccessKit node shows up in accessibility inspector but doesn't have an accessibility tree under it.
  • Windows: I'm building my app with mingw and I haven't figured out how to link to uiautomationcore.dll from mingw yet.
  • Linux: encountered the above crash. I'll try to work around it now that I know about the executor issue.

Speaking of winit, you should consider tweaking your objc2 version to allow the 0.3.0-beta.3.patch-leaks bugfix version, context here: rust-windowing/winit#2739

@mwcampbell
Copy link
Contributor

@lunixbochs I'd like to work with you to debug the macOS issue. Where would be a convenient place for us to chat about that, so we don't clutter this issue?

@lunixbochs
Copy link
Contributor Author

The Talon slack?

@mwcampbell
Copy link
Contributor

@lunixbochs OK, I'm on the Talon Slack now. I don't know how to find you there, but you should be able to DM me, or mention me on an appropriate channel if you prefer.

@DataTriny
Copy link
Member

Hi there,

I already suggested adding async-io and tokio features here.

But actually doing it is tricky. We rely on zbus blocking::Connection's executor to dispatch our events in a non-blocking fashion, but for the rest we rely on zbus blocking wrappers. What this mean in a tokio environment is that the event dispatching task will fail if Adapter::new is not called inside an async context, but doing so will impact all the other calls because the blocking wrappers try to spin their own runtime, which is not allowed inside another runtime.

thread 'main' panicked at 'Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.', /home/username/.cargo/registry/src/github.com-1ecc6299db9ec823/zbus-3.10.0/src/utils.rs:50:14

The easiest (and maybe cleanest) way to solve this I see right now would be to offer both a blocking and an async API. If we want to keep a purely blocking API, I don't see how we could detect wether the caller:

  • has started an async runtime, and so we should await futures until completion,
  • has not started an async runtime, so we should start our own or use zbus blocking API.

Providing both a blocking and an async API would require significant changes under the hood, but we don't have many public function so the scope is quite narrow.

@lunixbochs
Copy link
Contributor Author

lunixbochs commented Mar 14, 2023

Winit is currently a synchronous runloop, and the only way for me to really own a winit Window is on the winit main thread in the synchronous runloop. Even though I have tokio code, it's not on this thread. I suspect most windows are going to be managed on synchronous runloops for the same reason. So it might make the most sense for accesskit to run its own executor.

@lunixbochs
Copy link
Contributor Author

lunixbochs commented Apr 29, 2023

As a workaround, I think we should add a tokio feature that just runs zbus in a tokio single thread executor, so at least people enabling the zbus tokio feature can bypass the crash. Right now I'm avoiding the crash by just disabling accesskit on Linux, which is a strictly worse outcome than a dedicated executor.

I think the more complicated answer would be to make a public async api for accesskit so the user can manage this themselves, but I kind of doubt many people will want to provide an async executor for their synchronous window event loop.

@lunixbochs
Copy link
Contributor Author

lunixbochs commented May 9, 2023

Responding to this question from the PR:

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 make an app (foo). I use two crates (A, B) that internally use zbus.
Crate A uses the zbus blocking api.
Crate B exposes an async api using tokio, and enables the zbus tokio feature.
I call crate A from a sync thread. I call crate B from an async tokio runloop loop.

Using both crate A and crate B in this way at the same time, regardless of which one you were using first, panics on the synchronous thread inside zbus. This happens because the async crate B enables the zbus tokio feature, which assumes it's always safe to spawn tokio tasks inside zbus. You likely can't make synchronous calls into zbus if any crate in your entire tree enables the zbus tokio feature, but synchronous calls into zbus are safe otherwise!

That's what happened here, and it's clearly a zbus bug in my eyes.
zbus probably shouldn't be spawning tokio tasks from synchronous functions and just assuming they're in the middle of a tokio runloop.

GUI apps generally aren't going to be running UI code inside a tokio runloop, because they're already in something like a winit runloop. So zbus will just generally have this problem in UI code. That's a problem for accesskit, because accessibility is inherently a UI thing.

The zbus synchronous api with features=["tokio"] should probably assume it needs to manage its own tokio executor, and it should expose a separate async api if it's going to assume the functions are being called from inside an async runloop.

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 a pull request may close this issue.

3 participants