From 89d67507c7067ddbc65816192e74f771cdd0f91b Mon Sep 17 00:00:00 2001 From: alex-ds13 <145657253+alex-ds13@users.noreply.github.com> Date: Mon, 24 Feb 2025 01:50:53 +0000 Subject: [PATCH] fix(reaper): avoid deadlocks at startup Previously the reaper at startup would lock it's own `HWNDS_CACHE` and then try to lock the WM to get its `known_hwnds`. However if there was an event in the meantime, process_event would lock the WM first and then it would try to lock the reaper's `HWNDS_CACHE` to update it. This would deadlock since that would be locked by the reaper waiting for the WM lock to be released. This commit now makes it so we pass the `known_hwnds` to the reaper as an argument at startup and it also rearranges the order of loading the listeners. Now komorebi first loads all the manager-type listeners, and only afterwards does it load the commands and events listeners. After some testing this seems to be the best order that doesn't cause any issues at all! There were some other issues that I've noticed before when starting komorebi while having other 3rd parties trying to subscribe to it (like komorebi-bar and YASB), which would make those subscribers lock the `process_command` thread. This doesn't seem to be happening on my tests anymore with this new order. --- komorebi/src/main.rs | 26 +++++++++++++------------- komorebi/src/reaper.rs | 26 ++++++++------------------ 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/komorebi/src/main.rs b/komorebi/src/main.rs index 7d60d2af..a76d97f9 100644 --- a/komorebi/src/main.rs +++ b/komorebi/src/main.rs @@ -244,16 +244,10 @@ fn main() -> Result<()> { StaticConfig::postload(config, &wm)?; } - listen_for_commands(wm.clone()); - if !opts.await_configuration && !INITIAL_CONFIGURATION_LOADED.load(Ordering::SeqCst) { INITIAL_CONFIGURATION_LOADED.store(true, Ordering::SeqCst); }; - if let Some(port) = opts.tcp_port { - listen_for_commands_tcp(wm.clone(), port); - } - if static_config.is_none() { std::thread::spawn(|| load_configuration().expect("could not load configuration")); @@ -280,21 +274,27 @@ fn main() -> Result<()> { wm.lock().retile_all(false)?; - listen_for_events(wm.clone()); - - if CUSTOM_FFM.load(Ordering::SeqCst) { - listen_for_movements(wm.clone()); - } - border_manager::listen_for_notifications(wm.clone()); stackbar_manager::listen_for_notifications(wm.clone()); transparency_manager::listen_for_notifications(wm.clone()); workspace_reconciliator::listen_for_notifications(wm.clone()); monitor_reconciliator::listen_for_notifications(wm.clone())?; - reaper::listen_for_notifications(wm.clone()); + reaper::listen_for_notifications(wm.clone(), wm.lock().known_hwnds.clone()); focus_manager::listen_for_notifications(wm.clone()); theme_manager::listen_for_notifications(); + listen_for_commands(wm.clone()); + + if let Some(port) = opts.tcp_port { + listen_for_commands_tcp(wm.clone(), port); + } + + listen_for_events(wm.clone()); + + if CUSTOM_FFM.load(Ordering::SeqCst) { + listen_for_movements(wm.clone()); + } + let (ctrlc_sender, ctrlc_receiver) = crossbeam_channel::bounded(1); ctrlc::set_handler(move || { ctrlc_sender diff --git a/komorebi/src/reaper.rs b/komorebi/src/reaper.rs index ef919db7..f79771eb 100644 --- a/komorebi/src/reaper.rs +++ b/komorebi/src/reaper.rs @@ -47,8 +47,11 @@ pub fn send_notification(hwnds: HashMap) { } } -pub fn listen_for_notifications(wm: Arc>) { - watch_for_orphans(wm.clone()); +pub fn listen_for_notifications( + wm: Arc>, + known_hwnds: HashMap, +) { + watch_for_orphans(known_hwnds); std::thread::spawn(move || loop { match handle_notifications(wm.clone()) { @@ -70,25 +73,12 @@ fn handle_notifications(wm: Arc>) -> color_eyre::Result<()> for notification in receiver { let orphan_hwnds = notification.0; let mut wm = wm.lock(); - let offset = wm.work_area_offset; let mut update_borders = false; for (hwnd, (m_idx, w_idx)) in orphan_hwnds.iter() { if let Some(monitor) = wm.monitors_mut().get_mut(*m_idx) { let focused_workspace_idx = monitor.focused_workspace_idx(); - let work_area = *monitor.work_area_size(); - let window_based_work_area_offset = ( - monitor.window_based_work_area_offset_limit(), - monitor.window_based_work_area_offset(), - ); - - let offset = if monitor.work_area_offset().is_some() { - monitor.work_area_offset() - } else { - offset - }; - if let Some(workspace) = monitor.workspaces_mut().get_mut(*w_idx) { // Remove orphan window if let Err(error) = workspace.remove_window(*hwnd) { @@ -105,7 +95,7 @@ fn handle_notifications(wm: Arc>) -> color_eyre::Result<()> // If this is not a focused workspace there is no need to update the // workspace or the borders. That will already be done when the user // changes to this workspace. - workspace.update(&work_area, offset, window_based_work_area_offset)?; + workspace.update()?; update_borders = true; } tracing::info!( @@ -150,11 +140,11 @@ fn handle_notifications(wm: Arc>) -> color_eyre::Result<()> Ok(()) } -fn watch_for_orphans(wm: Arc>) { +fn watch_for_orphans(known_hwnds: HashMap) { // Cache current hwnds { let mut cache = HWNDS_CACHE.lock(); - *cache = wm.lock().known_hwnds.clone(); + *cache = known_hwnds; } std::thread::spawn(move || loop {