From 3349350bf6f3066e5e7ab7ad114138bba14d9b94 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Thu, 26 Mar 2026 16:39:10 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20robustness=20=E2=80=94=20RwLock=20for=20?= =?UTF-8?q?concurrent=20reads,=20log=20malformed=20JSON=20requests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace Mutex with RwLock for ProviderManager and FrecencyStore in the IPC server. Most request types (Query, Providers, Submenu, PluginAction) only need read access and can now proceed concurrently. Only Launch (frecency write) and Refresh (provider write) acquire exclusive locks. Also adds a warn!() log for malformed JSON requests before sending the error response, improving observability for debugging client issues. Provider trait now requires Send + Sync to satisfy RwLock's Sync bound on the inner type. RuntimeProvider and LuaProvider gain the corresponding unsafe impl Sync. --- .../owlry-core/src/plugins/runtime_loader.rs | 6 +++- .../owlry-core/src/providers/lua_provider.rs | 7 ++-- crates/owlry-core/src/providers/mod.rs | 2 +- crates/owlry-core/src/server.rs | 33 ++++++++++--------- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/crates/owlry-core/src/plugins/runtime_loader.rs b/crates/owlry-core/src/plugins/runtime_loader.rs index 3f91ea4..a8542e3 100644 --- a/crates/owlry-core/src/plugins/runtime_loader.rs +++ b/crates/owlry-core/src/plugins/runtime_loader.rs @@ -243,8 +243,12 @@ impl Provider for RuntimeProvider { } } -// RuntimeProvider needs to be Send for the Provider trait +// RuntimeProvider needs to be Send + Sync for the Provider trait. +// Safety: RuntimeHandle is an opaque FFI handle accessed only through +// extern "C" vtable functions. The same safety argument that justifies +// Send applies to Sync — all access is mediated by the vtable. unsafe impl Send for RuntimeProvider {} +unsafe impl Sync for RuntimeProvider {} /// Check if the Lua runtime is available pub fn lua_runtime_available() -> bool { diff --git a/crates/owlry-core/src/providers/lua_provider.rs b/crates/owlry-core/src/providers/lua_provider.rs index 675fcb8..0d64c6d 100644 --- a/crates/owlry-core/src/providers/lua_provider.rs +++ b/crates/owlry-core/src/providers/lua_provider.rs @@ -89,10 +89,11 @@ impl Provider for LuaProvider { } } -// LuaProvider needs to be Send for the Provider trait -// Since we're using Rc>, we need to be careful about thread safety -// For now, owlry is single-threaded, so this is safe +// LuaProvider needs to be Send + Sync for the Provider trait. +// Since we're using Rc>, we need to be careful about thread safety. +// For now, owlry is single-threaded, so this is safe. unsafe impl Send for LuaProvider {} +unsafe impl Sync for LuaProvider {} /// Create LuaProviders from all registered providers in a plugin pub fn create_providers_from_plugin(plugin: Rc>) -> Vec> { diff --git a/crates/owlry-core/src/providers/mod.rs b/crates/owlry-core/src/providers/mod.rs index 4e936d0..8591377 100644 --- a/crates/owlry-core/src/providers/mod.rs +++ b/crates/owlry-core/src/providers/mod.rs @@ -94,7 +94,7 @@ impl std::fmt::Display for ProviderType { } /// Trait for all search providers -pub trait Provider: Send { +pub trait Provider: Send + Sync { #[allow(dead_code)] fn name(&self) -> &str; fn provider_type(&self) -> ProviderType; diff --git a/crates/owlry-core/src/server.rs b/crates/owlry-core/src/server.rs index 7a8b8e6..7f5e105 100644 --- a/crates/owlry-core/src/server.rs +++ b/crates/owlry-core/src/server.rs @@ -2,7 +2,7 @@ use std::io::{self, BufRead, BufReader, Write}; use std::os::unix::fs::PermissionsExt; use std::os::unix::net::{UnixListener, UnixStream}; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, RwLock}; use std::time::Duration; use std::thread; @@ -22,8 +22,8 @@ use crate::providers::{LaunchItem, ProviderManager}; pub struct Server { listener: UnixListener, socket_path: PathBuf, - provider_manager: Arc>, - frecency: Arc>, + provider_manager: Arc>, + frecency: Arc>, config: Arc, } @@ -49,8 +49,8 @@ impl Server { Ok(Self { listener, socket_path: socket_path.to_path_buf(), - provider_manager: Arc::new(Mutex::new(provider_manager)), - frecency: Arc::new(Mutex::new(frecency)), + provider_manager: Arc::new(RwLock::new(provider_manager)), + frecency: Arc::new(RwLock::new(frecency)), config: Arc::new(config), }) } @@ -96,8 +96,8 @@ impl Server { /// dispatch each, and write the JSON response back. fn handle_client( stream: UnixStream, - pm: Arc>, - frecency: Arc>, + pm: Arc>, + frecency: Arc>, config: Arc, ) -> io::Result<()> { stream.set_read_timeout(Some(Duration::from_secs(30)))?; @@ -131,6 +131,7 @@ impl Server { let request: Request = match serde_json::from_str(trimmed) { Ok(req) => req, Err(e) => { + warn!("Malformed request from client: {}", e); let resp = Response::Error { message: format!("invalid request JSON: {}", e), }; @@ -150,8 +151,8 @@ impl Server { /// the response. fn handle_request( request: &Request, - pm: &Arc>, - frecency: &Arc>, + pm: &Arc>, + frecency: &Arc>, config: &Arc, ) -> Response { match request { @@ -163,8 +164,8 @@ impl Server { let max = config.general.max_results; let weight = config.providers.frecency_weight; - let pm_guard = pm.lock().unwrap_or_else(|e| e.into_inner()); - let frecency_guard = frecency.lock().unwrap_or_else(|e| e.into_inner()); + let pm_guard = pm.read().unwrap_or_else(|e| e.into_inner()); + let frecency_guard = frecency.read().unwrap_or_else(|e| e.into_inner()); let results = pm_guard.search_with_frecency( text, max, @@ -186,13 +187,13 @@ impl Server { item_id, provider: _, } => { - let mut frecency_guard = frecency.lock().unwrap_or_else(|e| e.into_inner()); + let mut frecency_guard = frecency.write().unwrap_or_else(|e| e.into_inner()); frecency_guard.record_launch(item_id); Response::Ack } Request::Providers => { - let pm_guard = pm.lock().unwrap_or_else(|e| e.into_inner()); + let pm_guard = pm.read().unwrap_or_else(|e| e.into_inner()); let descs = pm_guard.available_providers(); Response::Providers { list: descs.into_iter().map(descriptor_to_desc).collect(), @@ -200,7 +201,7 @@ impl Server { } Request::Refresh { provider } => { - let mut pm_guard = pm.lock().unwrap_or_else(|e| e.into_inner()); + let mut pm_guard = pm.write().unwrap_or_else(|e| e.into_inner()); pm_guard.refresh_provider(provider); Response::Ack } @@ -211,7 +212,7 @@ impl Server { } Request::Submenu { plugin_id, data } => { - let pm_guard = pm.lock().unwrap_or_else(|e| e.into_inner()); + let pm_guard = pm.read().unwrap_or_else(|e| e.into_inner()); match pm_guard.query_submenu_actions(plugin_id, data, plugin_id) { Some((_name, actions)) => Response::SubmenuItems { items: actions @@ -226,7 +227,7 @@ impl Server { } Request::PluginAction { command } => { - let pm_guard = pm.lock().unwrap_or_else(|e| e.into_inner()); + let pm_guard = pm.read().unwrap_or_else(|e| e.into_inner()); if pm_guard.execute_plugin_action(command) { Response::Ack } else {