diff --git a/.changes/global-shortcut-release-lock-before-handler.md b/.changes/global-shortcut-release-lock-before-handler.md new file mode 100644 index 000000000..7e59a8902 --- /dev/null +++ b/.changes/global-shortcut-release-lock-before-handler.md @@ -0,0 +1,6 @@ +--- +global-shortcut: patch +global-shortcut-js: patch +--- + +Release the shortcuts lock before running shortcut handlers, so a handler that calls back into the plugin, or pumps messages that dispatch another shortcut event, no longer deadlocks the app. diff --git a/plugins/global-shortcut/Cargo.toml b/plugins/global-shortcut/Cargo.toml index 9a244ee55..c2615e1ad 100644 --- a/plugins/global-shortcut/Cargo.toml +++ b/plugins/global-shortcut/Cargo.toml @@ -33,5 +33,8 @@ tauri = { workspace = true } log = { workspace = true } thiserror = { workspace = true } +[dev-dependencies] +tauri = { workspace = true, features = ["test"] } + [target."cfg(not(any(target_os = \"android\", target_os = \"ios\")))".dependencies] global-hotkey = { version = "0.8", features = ["serde"] } diff --git a/plugins/global-shortcut/src/lib.rs b/plugins/global-shortcut/src/lib.rs index f65c1de1a..64112fe8d 100644 --- a/plugins/global-shortcut/src/lib.rs +++ b/plugins/global-shortcut/src/lib.rs @@ -63,6 +63,32 @@ struct RegisteredShortcut { handler: Option>>, } +/// Runs the handlers registered for the shortcut behind `event`. +/// +/// The shortcut and its handler are copied out of the map and the lock is released before any +/// user code runs, so a handler can call back into the plugin (or pump messages that dispatch +/// another shortcut event) without deadlocking on `shortcuts`. +fn dispatch_shortcut_event( + shortcuts: &Mutex>>, + app: &AppHandle, + global_handler: Option<&HandlerFn>, + event: GlobalHotKeyEvent, +) { + let registered = shortcuts + .lock() + .unwrap() + .get(&event.id) + .map(|s| (s.shortcut, s.handler.clone())); + if let Some((shortcut, shortcut_handler)) = registered { + if let Some(handler) = &shortcut_handler { + handler(app, &shortcut, event); + } + if let Some(handler) = global_handler { + handler(app, &shortcut, event); + } + } +} + struct GlobalHotKeyManager(global_hotkey::GlobalHotKeyManager); /// SAFETY: we ensure it is run on main thread only @@ -475,14 +501,7 @@ impl Builder { let app_handle = app.clone(); GlobalHotKeyEvent::set_event_handler(Some(move |e: GlobalHotKeyEvent| { - if let Some(shortcut) = shortcuts_.lock().unwrap().get(&e.id) { - if let Some(handler) = &shortcut.handler { - handler(&app_handle, &shortcut.shortcut, e); - } - if let Some(handler) = &handler { - handler(&app_handle, &shortcut.shortcut, e); - } - } + dispatch_shortcut_event(&shortcuts_, &app_handle, handler.as_ref(), e); })); app.manage(GlobalShortcut { @@ -495,3 +514,64 @@ impl Builder { .build() } } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + use tauri::test::{MockRuntime, mock_app}; + + #[test] + fn handlers_run_without_holding_the_shortcuts_lock() { + let app = mock_app(); + let shortcut = Shortcut::new(Some(Modifiers::ALT), Code::KeyW); + let shortcuts = Arc::new(Mutex::new(HashMap::< + HotKeyId, + RegisteredShortcut, + >::new())); + let calls = Arc::new(AtomicUsize::new(0)); + + let shortcut_handler: HandlerFn = { + let shortcuts = shortcuts.clone(); + let calls = calls.clone(); + Box::new(move |_, _, _| { + assert!( + shortcuts.try_lock().is_ok(), + "the shortcuts lock must not be held while a handler runs" + ); + calls.fetch_add(1, Ordering::SeqCst); + }) + }; + let global_handler: HandlerFn = { + let shortcuts = shortcuts.clone(); + let calls = calls.clone(); + Box::new(move |_, _, _| { + assert!( + shortcuts.try_lock().is_ok(), + "the shortcuts lock must not be held while a handler runs" + ); + calls.fetch_add(1, Ordering::SeqCst); + }) + }; + + shortcuts.lock().unwrap().insert( + shortcut.id(), + RegisteredShortcut { + shortcut, + handler: Some(Arc::new(shortcut_handler)), + }, + ); + + dispatch_shortcut_event( + &shortcuts, + app.handle(), + Some(&global_handler), + GlobalHotKeyEvent { + id: shortcut.id(), + state: ShortcutState::Pressed, + }, + ); + + assert_eq!(calls.load(Ordering::SeqCst), 2); + } +}