From 8b609cc5b451f8c8b7e16bc782657983204565f6 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Tue, 7 Sep 2021 10:43:21 +0200 Subject: [PATCH 01/10] two options for creating error callbacks --- Cargo.toml | 1 + src/config.rs | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 8db9ec8f..75af014f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,6 +24,7 @@ gdal-sys = { path = "gdal-sys", version = "^0.5"} ndarray = {version = "0.15", optional = true } chrono = { version = "0.4", optional = true } bitflags = "1.2" +lazy_static = "1.3" [build-dependencies] gdal-sys = { path = "gdal-sys", version= "^0.5"} diff --git a/src/config.rs b/src/config.rs index fdbf3556..b0107707 100644 --- a/src/config.rs +++ b/src/config.rs @@ -23,9 +23,14 @@ //! Refer to [GDAL `ConfigOptions`](https://trac.osgeo.org/gdal/wiki/ConfigOptions) for //! a full list of options. +use gdal_sys::{CPLErr, CPLErrorNum, CPLGetErrorHandlerUserData}; +use libc::{c_char, c_void}; + use crate::errors::Result; use crate::utils::_string; +use lazy_static::lazy_static; use std::ffi::CString; +use std::sync::Mutex; /// Set a GDAL library configuration option /// @@ -108,10 +113,203 @@ pub fn clear_thread_local_config_option(key: &str) -> Result<()> { Ok(()) } +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +#[repr(C)] +pub enum CplErr { + None = 0, + Debug = 1, + Warning = 2, + Failure = 3, + Fatal = 4, +} + +type CallbackType = dyn FnMut(CplErr, i32, &str) + 'static; +static mut ERROR_CALLBACK: Option> = None; + +type CallbackTypeThreadSafe = dyn FnMut(CplErr, i32, &str) + 'static + Send; +lazy_static! { + static ref ERROR_CALLBACK_THREAD_SAFE: Mutex>> = + Mutex::new(None); +} + +/// 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`]. +/// 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, +{ + 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); + } + + unsafe { + // this part is not thread safe + ERROR_CALLBACK.replace(Box::new(callback)); + + if let Some(callback) = &mut ERROR_CALLBACK { + gdal_sys::CPLSetErrorHandlerEx(Some(error_handler), callback 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() { + 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); + }; + } +} + +/// Remove a custom error handler for GDAL. +pub fn remove_error_handler() { + unsafe { + 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() { + Ok(guard) => guard, + // poor man's lock poisoning handling, i.e., ignoring it + Err(poison_error) => poison_error.into_inner(), + }; + + callback_lock.take(); +} + #[cfg(test)] mod tests { + + use std::sync::{Arc, Mutex}; + use super::*; + #[test] + fn error_handler() { + let errors: Arc>> = Arc::new(Mutex::new(vec![])); + + let errors_clone = errors.clone(); + + set_error_handler(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(); + + 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 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()); From da4c2acf8944777e634eb33637d985bda1de278f Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Tue, 7 Sep 2021 11:41:22 +0200 Subject: [PATCH 02/10] changed lazy_static to once_cell --- Cargo.toml | 2 +- src/config.rs | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 75af014f..2520d9ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ gdal-sys = { path = "gdal-sys", version = "^0.5"} ndarray = {version = "0.15", optional = true } chrono = { version = "0.4", optional = true } bitflags = "1.2" -lazy_static = "1.3" +once_cell = "1.8" [build-dependencies] gdal-sys = { path = "gdal-sys", version= "^0.5"} diff --git a/src/config.rs b/src/config.rs index b0107707..18e84ee3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -28,7 +28,7 @@ use libc::{c_char, c_void}; use crate::errors::Result; use crate::utils::_string; -use lazy_static::lazy_static; +use once_cell::sync::Lazy; use std::ffi::CString; use std::sync::Mutex; @@ -127,10 +127,9 @@ type CallbackType = dyn FnMut(CplErr, i32, &str) + 'static; static mut ERROR_CALLBACK: Option> = None; type CallbackTypeThreadSafe = dyn FnMut(CplErr, i32, &str) + 'static + Send; -lazy_static! { - static ref ERROR_CALLBACK_THREAD_SAFE: Mutex>> = - Mutex::new(None); -} + +static ERROR_CALLBACK_THREAD_SAFE: Lazy>>> = + Lazy::new(Default::default); /// Set a custom error handler for GDAL. /// Could be overwritten by setting a thread-local error handler. From 0111fbd1f59549ed10543aeb4e507e8081578903 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Wed, 8 Sep 2021 16:09:38 +0200 Subject: [PATCH 03/10] 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()); From 6387a298ad3be5f321e3bec040b7f94d40245caf Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Wed, 8 Sep 2021 16:19:17 +0200 Subject: [PATCH 04/10] move CplErrorType to error module --- src/config.rs | 28 +++++++++------------------- src/errors.rs | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/config.rs b/src/config.rs index d65eae5f..d5887e76 100644 --- a/src/config.rs +++ b/src/config.rs @@ -26,7 +26,7 @@ use gdal_sys::{CPLErr, CPLErrorNum, CPLGetErrorHandlerUserData}; use libc::{c_char, c_void}; -use crate::errors::Result; +use crate::errors::{CplErrType, Result}; use crate::utils::_string; use once_cell::sync::Lazy; use std::ffi::CString; @@ -114,17 +114,7 @@ pub fn clear_thread_local_config_option(key: &str) -> Result<()> { Ok(()) } -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -#[repr(C)] -pub enum CplErr { - None = 0, - Debug = 1, - Warning = 2, - Failure = 3, - Fatal = 4, -} - -type CallbackType = dyn FnMut(CplErr, i32, &str) + 'static + Send; +type CallbackType = dyn FnMut(CplErrType, i32, &str) + 'static + Send; type PinnedCallback = Pin>>; /// Static variable that holds the current error callback function @@ -139,7 +129,7 @@ static ERROR_CALLBACK: Lazy>> = Lazy::new(Default:: /// pub fn set_error_handler(callback: F) where - F: FnMut(CplErr, i32, &str) + 'static + Send, + F: FnMut(CplErrType, i32, &str) + 'static + Send, { unsafe extern "C" fn error_handler( error_type: CPLErr::Type, @@ -147,7 +137,7 @@ where error_msg_ptr: *const c_char, ) { let error_msg = _string(error_msg_ptr); - let error_type: CplErr = std::mem::transmute(error_type); + let error_type: CplErrType = error_type.into(); // reconstruct callback from user data pointer let callback_raw = CPLGetErrorHandlerUserData(); @@ -199,7 +189,7 @@ mod tests { #[test] fn error_handler() { - let errors: Arc>> = Arc::new(Mutex::new(vec![])); + let errors: Arc>> = Arc::new(Mutex::new(vec![])); let errors_clone = errors.clone(); @@ -214,17 +204,17 @@ mod tests { unsafe { let msg = CString::new("bar".as_bytes()).unwrap(); - gdal_sys::CPLError(std::mem::transmute(CplErr::Warning), 1, msg.as_ptr()); + gdal_sys::CPLError(std::mem::transmute(CplErrType::Warning), 1, msg.as_ptr()); }; remove_error_handler(); - let result: Vec<(CplErr, i32, String)> = errors.lock().unwrap().clone(); + let result: Vec<(CplErrType, i32, String)> = errors.lock().unwrap().clone(); assert_eq!( result, vec![ - (CplErr::Failure, 42, "foo".to_string()), - (CplErr::Warning, 1, "bar".to_string()) + (CplErrType::Failure, 42, "foo".to_string()), + (CplErrType::Warning, 1, "bar".to_string()) ] ); } diff --git a/src/errors.rs b/src/errors.rs index 35bdd877..e15a618b 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -69,3 +69,24 @@ pub enum GdalError { #[error("Unable to unlink mem file: {file_name}")] UnlinkMemFile { file_name: String }, } + +/// A wrapper for [`CPLErr::Type`] that reflects it as an enum +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +#[repr(C)] +pub enum CplErrType { + None = 0, + Debug = 1, + Warning = 2, + Failure = 3, + Fatal = 4, +} + +impl From for CplErrType { + fn from(error_type: CPLErr::Type) -> Self { + if error_type > 4 { + return Self::None; // fallback type, should not happen + } + + unsafe { std::mem::transmute(error_type) } + } +} From bce691d29b880f1761266122ab45ea9f65d52ca4 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Fri, 10 Sep 2021 15:08:36 +0200 Subject: [PATCH 05/10] removing error callback race-condition --- src/config.rs | 53 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/src/config.rs b/src/config.rs index d5887e76..634a0226 100644 --- a/src/config.rs +++ b/src/config.rs @@ -150,33 +150,36 @@ where let mut callback: PinnedCallback = Box::pin(Box::new(callback)); 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); - }; 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(), }; + + // changing the error callback is fenced by the callback lock + unsafe { + gdal_sys::CPLSetErrorHandlerEx(Some(error_handler), callback_ref 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. pub fn remove_error_handler() { - unsafe { - gdal_sys::CPLSetErrorHandler(None); - }; - - // drop callback - 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(), }; + // changing the error callback is fenced by the callback lock + unsafe { + gdal_sys::CPLSetErrorHandler(None); + }; + + // drop callback callback_lock.take(); } @@ -219,6 +222,38 @@ mod tests { ); } + #[test] + fn error_handler_interleaved() { + use std::thread; + // Two racing threads trying to set error handlers + // First one + thread::spawn(move || loop { + set_error_handler(move |_a, _b, _c| {}); + }); + + // Second one + thread::spawn(move || loop { + set_error_handler(move |_a, _b, _c| {}); + }); + + // A thread that makes a lot of mistakes + let join_handle = thread::spawn(move || { + for _ in 0..100 { + 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(CplErrType::Warning), 1, msg.as_ptr()); + }; + } + }); + + join_handle.join().unwrap(); + } + #[test] fn test_set_get_option() { assert!(set_config_option("GDAL_CACHEMAX", "128").is_ok()); From 40e7fc8256cf8cf656ba4ac7f28c318d7e4524b4 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Fri, 10 Sep 2021 15:13:04 +0200 Subject: [PATCH 06/10] fixing doc-comments --- src/config.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/config.rs b/src/config.rs index 634a0226..c99674f9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -114,19 +114,19 @@ pub fn clear_thread_local_config_option(key: &str) -> Result<()> { Ok(()) } -type CallbackType = dyn FnMut(CplErrType, i32, &str) + 'static + Send; -type PinnedCallback = Pin>>; +type ErrorCallbackType = dyn FnMut(CplErrType, i32, &str) + 'static + Send; +type PinnedErrorCallback = Pin>>; /// Static variable that holds the current error callback function -static ERROR_CALLBACK: Lazy>> = Lazy::new(Default::default); +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. /// -/// Note: -/// Stores the callback in the static variable [`ERROR_CALLBACK_THREAD_SAFE`]. -/// Internally, it passes a pointer to the callback to GDAL as `pUserData`. -/// +// Note: +// Stores the callback in the static variable [`ERROR_CALLBACK`]. +// Internally, it passes a pointer to the callback to GDAL as `pUserData`. +// pub fn set_error_handler(callback: F) where F: FnMut(CplErrType, i32, &str) + 'static + Send, @@ -141,15 +141,15 @@ where // reconstruct callback from user data pointer let callback_raw = CPLGetErrorHandlerUserData(); - let callback: &mut Box = &mut *(callback_raw as *mut Box<_>); + let callback: &mut Box = &mut *(callback_raw as *mut Box<_>); callback(error_type, error_num as i32, &error_msg); } // pin memory location of callback for sending its pointer to GDAL - let mut callback: PinnedCallback = Box::pin(Box::new(callback)); + let mut callback: PinnedErrorCallback = Box::pin(Box::new(callback)); - let callback_ref: &mut Box = callback.as_mut().get_mut(); + let callback_ref: &mut Box = callback.as_mut().get_mut(); let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, From 4e5e45b97db0ab845b471fd5e9b07ffdc89d4bee Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Fri, 10 Sep 2021 15:16:13 +0200 Subject: [PATCH 07/10] not pinning the box-fn box --- src/config.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index c99674f9..b3f27177 100644 --- a/src/config.rs +++ b/src/config.rs @@ -30,7 +30,6 @@ use crate::errors::{CplErrType, 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 @@ -115,7 +114,7 @@ pub fn clear_thread_local_config_option(key: &str) -> Result<()> { } type ErrorCallbackType = dyn FnMut(CplErrType, i32, &str) + 'static + Send; -type PinnedErrorCallback = Pin>>; +type PinnedErrorCallback = Box>; /// Static variable that holds the current error callback function static ERROR_CALLBACK: Lazy>> = Lazy::new(Default::default); @@ -147,9 +146,9 @@ where } // pin memory location of callback for sending its pointer to GDAL - let mut callback: PinnedErrorCallback = Box::pin(Box::new(callback)); + let mut callback: PinnedErrorCallback = Box::new(Box::new(callback)); - let callback_ref: &mut Box = callback.as_mut().get_mut(); + let callback_ref: &mut Box = callback.as_mut(); let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, From bd4beaa76d6f1cbb5f0283e832b867cdf34118de Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Tue, 14 Sep 2021 17:00:22 +0200 Subject: [PATCH 08/10] clarifying error callback handling --- src/config.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index b3f27177..375aa5cd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -126,9 +126,11 @@ static ERROR_CALLBACK: Lazy>> = Lazy::new(Defa // Stores the callback in the static variable [`ERROR_CALLBACK`]. // Internally, it passes a pointer to the callback to GDAL as `pUserData`. // +/// The function must be `Send` and `Sync` since it is potentially called from multiple threads. +/// pub fn set_error_handler(callback: F) where - F: FnMut(CplErrType, i32, &str) + 'static + Send, + F: FnMut(CplErrType, i32, &str) + 'static + Send + Sync, { unsafe extern "C" fn error_handler( error_type: CPLErr::Type, @@ -152,7 +154,7 @@ where let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, - // poor man's lock poisoning handling, i.e., ignoring it + // poisoning could only occur on `CPLSetErrorHandler(Ex)` panicing, thus the value must be valid nevertheless Err(poison_error) => poison_error.into_inner(), }; @@ -169,7 +171,7 @@ where pub fn remove_error_handler() { let mut callback_lock = match ERROR_CALLBACK.lock() { Ok(guard) => guard, - // poor man's lock poisoning handling, i.e., ignoring it + // poisoning could only occur on `CPLSetErrorHandler(Ex)` panicing, thus the value must be valid nevertheless Err(poison_error) => poison_error.into_inner(), }; @@ -235,7 +237,7 @@ mod tests { set_error_handler(move |_a, _b, _c| {}); }); - // A thread that makes a lot of mistakes + // A thread that provokes potential race conditions let join_handle = thread::spawn(move || { for _ in 0..100 { unsafe { From a4df45354521524982d8290a3db3eb5c7bda02af Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Thu, 16 Sep 2021 13:37:13 +0200 Subject: [PATCH 09/10] added change to CHANGES.md --- CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index e50e6ace..2f9ad8e6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -203,6 +203,9 @@ let mut dataset = driver - +- Added `set_error_handler` and `remove_error_handler` to the config module that wraps `CPLSetErrorHandlerEx` + + - ## 0.7.1 From 44db095d80fa731e5bd749376a094affa1bdcf01 Mon Sep 17 00:00:00 2001 From: Christian Beilschmidt Date: Thu, 16 Sep 2021 16:31:41 +0200 Subject: [PATCH 10/10] explanation for double-box --- src/config.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/config.rs b/src/config.rs index 375aa5cd..a5dc9ee7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -114,6 +114,10 @@ pub fn clear_thread_local_config_option(key: &str) -> Result<()> { } type ErrorCallbackType = dyn FnMut(CplErrType, i32, &str) + 'static + Send; +// We have to double-`Box` the type because we need two things: +// 1. A stable pointer for moving the data in and out of the `Mutex`. This is done by the outer `Box`. +// 2. A thin pointer to our Trait-`FnMut`. This is done by the inner (sized) `Box`. We cannot use `*mut dyn FnMut` +// (a fat pointer) since we have to cast it from a `*mut c_void`, which is a thin pointer. type PinnedErrorCallback = Box>; /// Static variable that holds the current error callback function