From 0111fbd1f59549ed10543aeb4e507e8081578903 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Wed, 8 Sep 2021 16:09:38 +0200 Subject: [PATCH] thread-safe error handler --- src/config.rs | 112 ++++++++------------------------------------------ 1 file changed, 16 insertions(+), 96 deletions(-) diff --git a/src/config.rs b/src/config.rs index 18e84ee3..d65eae5f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -30,6 +30,7 @@ use crate::errors::Result; use crate::utils::_string; use once_cell::sync::Lazy; use std::ffi::CString; +use std::pin::Pin; use std::sync::Mutex; /// Set a GDAL library configuration option @@ -123,26 +124,22 @@ pub enum CplErr { Fatal = 4, } -type CallbackType = dyn FnMut(CplErr, i32, &str) + 'static; -static mut ERROR_CALLBACK: Option> = None; +type CallbackType = dyn FnMut(CplErr, i32, &str) + 'static + Send; +type PinnedCallback = Pin>>; -type CallbackTypeThreadSafe = dyn FnMut(CplErr, i32, &str) + 'static + Send; - -static ERROR_CALLBACK_THREAD_SAFE: Lazy>>> = - Lazy::new(Default::default); +/// Static variable that holds the current error callback function +static ERROR_CALLBACK: Lazy>> = Lazy::new(Default::default); /// Set a custom error handler for GDAL. /// Could be overwritten by setting a thread-local error handler. /// -/// *This method is not thread safe.* -/// /// Note: -/// Stores the callback in the static variable [`ERROR_CALLBACK`]. +/// Stores the callback in the static variable [`ERROR_CALLBACK_THREAD_SAFE`]. /// Internally, it passes a pointer to the callback to GDAL as `pUserData`. /// pub fn set_error_handler(callback: F) where - F: FnMut(CplErr, i32, &str) + 'static, + F: FnMut(CplErr, i32, &str) + 'static + Send, { unsafe extern "C" fn error_handler( error_type: CPLErr::Type, @@ -159,54 +156,21 @@ where callback(error_type, error_num as i32, &error_msg); } - unsafe { - // this part is not thread safe - ERROR_CALLBACK.replace(Box::new(callback)); + // pin memory location of callback for sending its pointer to GDAL + let mut callback: PinnedCallback = Box::pin(Box::new(callback)); - if let Some(callback) = &mut ERROR_CALLBACK { - gdal_sys::CPLSetErrorHandlerEx(Some(error_handler), callback as *mut _ as *mut c_void); - } + let callback_ref: &mut Box = callback.as_mut().get_mut(); + unsafe { + gdal_sys::CPLSetErrorHandlerEx(Some(error_handler), callback_ref as *mut _ as *mut c_void); }; -} -/// Set a custom error handler for GDAL. -/// Could be overwritten by setting a thread-local error handler. -/// -/// Note: -/// Stores the callback in the static variable [`ERROR_CALLBACK_THREAD_SAFE`]. -/// Internally, it passes a pointer to the callback to GDAL as `pUserData`. -/// -pub fn set_error_handler_thread_safe(callback: F) -where - F: FnMut(CplErr, i32, &str) + 'static + Send, -{ - unsafe extern "C" fn error_handler( - error_type: CPLErr::Type, - error_num: CPLErrorNum, - error_msg_ptr: *const c_char, - ) { - let error_msg = _string(error_msg_ptr); - let error_type: CplErr = std::mem::transmute(error_type); - - // reconstruct callback from user data pointer - let callback_raw = CPLGetErrorHandlerUserData(); - let callback: &mut Box = &mut *(callback_raw as *mut Box<_>); - - callback(error_type, error_num as i32, &error_msg); - } - - let mut callback_lock = match ERROR_CALLBACK_THREAD_SAFE.lock() { + let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, // poor man's lock poisoning handling, i.e., ignoring it Err(poison_error) => poison_error.into_inner(), }; - callback_lock.replace(Box::new(callback)); - - if let Some(callback) = callback_lock.as_mut() { - unsafe { - gdal_sys::CPLSetErrorHandlerEx(Some(error_handler), callback as *mut _ as *mut c_void); - }; - } + // store callback in static variable so we avoid a dangling pointer + callback_lock.replace(callback); } /// Remove a custom error handler for GDAL. @@ -215,21 +179,9 @@ pub fn remove_error_handler() { gdal_sys::CPLSetErrorHandler(None); }; - // drop callback - unsafe { - ERROR_CALLBACK.take(); - } -} - -/// Remove a custom error handler for GDAL. -pub fn remove_error_handler_thread_safe() { - unsafe { - gdal_sys::CPLSetErrorHandler(None); - }; - // drop callback - let mut callback_lock = match ERROR_CALLBACK_THREAD_SAFE.lock() { + let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, // poor man's lock poisoning handling, i.e., ignoring it Err(poison_error) => poison_error.into_inner(), @@ -277,38 +229,6 @@ mod tests { ); } - #[test] - fn error_handler_thread_safe() { - let errors: Arc>> = Arc::new(Mutex::new(vec![])); - - let errors_clone = errors.clone(); - - set_error_handler_thread_safe(move |a, b, c| { - errors_clone.lock().unwrap().push((a, b, c.to_string())); - }); - - unsafe { - let msg = CString::new("foo".as_bytes()).unwrap(); - gdal_sys::CPLError(CPLErr::CE_Failure, 42, msg.as_ptr()); - }; - - unsafe { - let msg = CString::new("bar".as_bytes()).unwrap(); - gdal_sys::CPLError(std::mem::transmute(CplErr::Warning), 1, msg.as_ptr()); - }; - - remove_error_handler_thread_safe(); - - let result: Vec<(CplErr, i32, String)> = errors.lock().unwrap().clone(); - assert_eq!( - result, - vec![ - (CplErr::Failure, 42, "foo".to_string()), - (CplErr::Warning, 1, "bar".to_string()) - ] - ); - } - #[test] fn test_set_get_option() { assert!(set_config_option("GDAL_CACHEMAX", "128").is_ok());