More explicit error handling; bump to v0.2.14

Handle cases where constructing the `Mutex` object can fail, and push the related error through the system. Replace manual `Display` implementations with `thiserror` usage.
LiberTEM · Oct 1, 2024 · 8da6ffc · 8da6ffc
1 parent a7ca811
commit 8da6ffc
Show file tree

Hide file tree

Showing 28 changed files with 303 additions and 266 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/common/Cargo.toml b/common/Cargo.toml
@@ -2,7 +2,7 @@
 name = "common"
 authors = ["Alexander Clausen <[email protected]>"]
 license = "MIT"
-version = "0.2.13"
+version = "0.2.14"
 edition = "2021"
 rust-version = "1.71"
 
@@ -34,3 +34,6 @@ criterion = "0.5.1"
 [[bench]]
 name = "casting"
 harness = false
+
+[lints.rust]
+unused_must_use = "deny"
diff --git a/common/src/frame_stack.rs b/common/src/frame_stack.rs
@@ -1,6 +1,6 @@
 use std::fmt::Debug;
 
-use ipc_test::{SharedSlabAllocator, SlotForWriting};
+use ipc_test::{slab::ShmError, SharedSlabAllocator, SlotForWriting};
 use log::{error, warn};
 use pyo3::{
     exceptions::{PyRuntimeError, PyValueError},
@@ -45,6 +45,9 @@ pub enum FrameStackWriteError {
 
     #[error("too small")]
     TooSmall,
+
+    #[error("SHM access error: {0}")]
+    ShmAccessError(#[from] ShmError),
 }
 
 impl From<FrameStackWriteError> for PyErr {
@@ -56,6 +59,7 @@ impl From<FrameStackWriteError> for PyErr {
             FrameStackWriteError::TooSmall => {
                 PyValueError::new_err("frame stack too small to handle single frame")
             }
+            FrameStackWriteError::ShmAccessError(e) => PyValueError::new_err(e.to_string()),
         }
     }
 }
@@ -64,6 +68,9 @@ impl From<FrameStackWriteError> for PyErr {
 pub enum SplitError<M: FrameMeta> {
     #[error("shm full")]
     ShmFull(FrameStackHandle<M>),
+
+    #[error("shm access error: {0}")]
+    AccessError(#[from] ShmError),
 }
 
 pub struct FrameStackForWriting<M>
@@ -160,7 +167,7 @@ where
     ) -> Result<FrameStackHandle<M>, FrameStackWriteError> {
         if self.is_empty() {
             let slot_info = shm.writing_done(self.slot);
-            shm.free_idx(slot_info.slot_idx);
+            shm.free_idx(slot_info.slot_idx)?;
             return Err(FrameStackWriteError::Empty);
         }
 
@@ -180,7 +187,7 @@ where
     ) -> Result<(), FrameStackWriteError> {
         if self.is_empty() {
             let slot_info = shm.writing_done(self.slot);
-            shm.free_idx(slot_info.slot_idx);
+            shm.free_idx(slot_info.slot_idx)?;
             Ok(())
         } else {
             Err(FrameStackWriteError::NonEmpty)
@@ -296,15 +303,17 @@ mod inner {
                 let mut slot_left = match shm.try_get_mut() {
                     Ok(s) => s,
                     Err(ShmError::NoSlotAvailable) => return Err(SplitError::ShmFull(self)),
+                    Err(e @ ShmError::MutexError(_)) => return Err(e.into()),
                 };
                 let mut slot_right = match shm.try_get_mut() {
                     Ok(s) => s,
                     Err(ShmError::NoSlotAvailable) => {
                         // don't leak the left slot!
                         let l = shm.writing_done(slot_left);
-                        shm.free_idx(l.slot_idx);
+                        shm.free_idx(l.slot_idx)?;
                         return Err(SplitError::ShmFull(self));
                     }
+                    Err(e @ ShmError::MutexError(_)) => return Err(e.into()),
                 };
 
                 let slice_left = slot_left.as_slice_mut();
@@ -316,7 +325,7 @@ mod inner {
                 let left = shm.writing_done(slot_left);
                 let right = shm.writing_done(slot_right);
 
-                shm.free_idx(self.slot.slot_idx);
+                shm.free_idx(self.slot.slot_idx)?;
 
                 (left, right)
             };
@@ -351,8 +360,8 @@ mod inner {
             f(&slot_r)
         }
 
-        pub fn free_slot(self, shm: &mut SharedSlabAllocator) {
-            shm.free_idx(self.slot.slot_idx);
+        pub fn free_slot(self, shm: &mut SharedSlabAllocator) -> Result<(), ShmError> {
+            shm.free_idx(self.slot.slot_idx)
         }
     }
 
@@ -493,7 +502,7 @@ impl<'b, M: FrameMeta> Drop for WriteGuard<'b, M> {
                 match frame_stack.writing_done(self.shm) {
                     Ok(frame_stack) => {
                         warn!("discarding non-empty frame stack as result of previous errors");
-                        frame_stack.free_slot(self.shm);
+                        let _ = frame_stack.free_slot(self.shm);
                     }
                     Err(e) => error!("WriteGuard::drop failed: {e:?}"),
                 }
@@ -616,12 +625,12 @@ mod tests {
         assert_eq!(a.offsets.len() + b.offsets.len(), 2);
 
         // when the split is done, there should be one free shm slot:
-        assert_eq!(shm.num_slots_free(), 1);
+        assert_eq!(shm.num_slots_free().unwrap(), 1);
 
         // and we can free them again:
-        shm.free_idx(a.slot.slot_idx);
-        shm.free_idx(b.slot.slot_idx);
+        shm.free_idx(a.slot.slot_idx).unwrap();
+        shm.free_idx(b.slot.slot_idx).unwrap();
 
-        assert_eq!(shm.num_slots_free(), 3);
+        assert_eq!(shm.num_slots_free().unwrap(), 3);
     }
 }
diff --git a/common/src/generic_cam_client.rs b/common/src/generic_cam_client.rs
@@ -1,6 +1,9 @@
 use std::fmt::Debug;
 
-use ipc_test::{slab::SlabInitError, SharedSlabAllocator};
+use ipc_test::{
+    slab::{ShmError, SlabInitError},
+    SharedSlabAllocator,
+};
 use multiversion::multiversion;
 use ndarray::ArrayViewMut3;
 use num::cast::AsPrimitive;
@@ -18,6 +21,9 @@ pub enum CamClientError {
         error: SlabInitError,
     },
 
+    #[error("failed to access SHM: {0}")]
+    ShmError(#[from] ShmError),
+
     #[error("operation on closed client")]
     Closed,
 
@@ -166,7 +172,7 @@ where
         M: FrameMeta,
     {
         let shm = self.get_shm_mut()?;
-        handle.free_slot(shm);
+        handle.free_slot(shm)?;
         Ok(())
     }
 

diff --git a/common/src/generic_connection.rs b/common/src/generic_connection.rs
@@ -6,7 +6,10 @@ use std::{
     time::{Duration, Instant},
 };
 
-use ipc_test::{slab::SlabInitError, SharedSlabAllocator};
+use ipc_test::{
+    slab::{ShmError, SlabInitError},
+    SharedSlabAllocator,
+};
 use log::{debug, info, trace, warn};
 use stats::Stats;
 
@@ -53,6 +56,9 @@ pub enum ConnectionError {
     #[error("could not connect to SHM area: {0}")]
     ShmConnectError(#[from] SlabInitError),
 
+    #[error("could not access SHM")]
+    ShmAccessError(#[from] ShmError),
+
     #[error("background thread is dead")]
     Disconnected,
 
@@ -117,6 +123,7 @@ where
                 frame_stack = old_frame_stack;
                 continue;
             }
+            Err(SplitError::AccessError(e)) => return Err(e.into()),
         };
     }
 }
@@ -314,13 +321,13 @@ where
                     return Err(ConnectionError::FatalError(error))
                 }
                 ReceiverMsg::FrameStack { frame_stack } => {
-                    frame_stack.free_slot(&mut self.shm);
+                    frame_stack.free_slot(&mut self.shm)?;
                     return Err(ConnectionError::UnexpectedMessage(
                         "ReceiverMsg::FrameStack in wait_for_arm".to_owned(),
                     ));
                 }
                 ReceiverMsg::Finished { frame_stack } => {
-                    frame_stack.free_slot(&mut self.shm);
+                    frame_stack.free_slot(&mut self.shm)?;
                     return Err(ConnectionError::UnexpectedMessage(
                         "ReceiverMsg::Finished in wait_for_arm".to_owned(),
                     ));
@@ -406,11 +413,11 @@ where
             match res {
                 ReceiverMsg::FrameStack { frame_stack } => {
                     trace!("wait_for_status: ignoring received FrameStackHandle");
-                    frame_stack.free_slot(&mut self.shm);
+                    frame_stack.free_slot(&mut self.shm)?;
                 }
                 ReceiverMsg::Finished { frame_stack } => {
                     warn!("wait_for_status: ignoring FrameStackHandle received in ReceiverMsg::Finished message");
-                    frame_stack.free_slot(&mut self.shm);
+                    frame_stack.free_slot(&mut self.shm)?;
                 }
                 ReceiverMsg::FatalError { error } => {
                     return Err(ConnectionError::FatalError(error));
@@ -568,12 +575,13 @@ where
         self.wait_for_status(ConnectionStatus::Idle, *timeout, periodic_callback)
     }
 
-    pub fn log_shm_stats(&self) {
+    pub fn log_shm_stats(&self) -> Result<(), ConnectionError> {
         let shm = &self.shm;
-        let free = shm.num_slots_free();
+        let free = shm.num_slots_free()?;
         let total = shm.num_slots_total();
         self.stats.log_stats();
         info!("shm stats free/total: {}/{}", free, total);
+        Ok(())
     }
 
     pub fn reset_stats(&mut self) {

diff --git a/common/src/py_connection.rs b/common/src/py_connection.rs
@@ -191,7 +191,9 @@ macro_rules! impl_py_connection {
                     let _trace_guard = span_from_py(py, &format!("{}::close", stringify!($name)))?;
 
                     if let Some(mut conn_impl) = self.conn_impl.take() {
-                        conn_impl.log_shm_stats();
+                        conn_impl
+                            .log_shm_stats()
+                            .map_err(|e| PyConnectionError::new_err(e.to_string()))?;
                         conn_impl.reset_stats();
                         conn_impl.close();
                         Ok(())
@@ -261,7 +263,9 @@ macro_rules! impl_py_connection {
 
                 pub fn log_shm_stats(&self) -> PyResult<()> {
                     let conn_impl = self.get_conn()?;
-                    conn_impl.log_shm_stats();
+                    conn_impl
+                        .log_shm_stats()
+                        .map_err(|e| PyConnectionError::new_err(e.to_string()))?;
                     Ok(())
                 }
             }

diff --git a/ipc_test/Cargo.toml b/ipc_test/Cargo.toml
@@ -29,3 +29,6 @@ sendfd = "0.4.3"
 
 [target.'cfg(not(windows))'.dependencies]
 nix = { version = "0.29.0", features = ["poll"] }
+
+[lints.rust]
+unused_must_use = "deny"
diff --git a/ipc_test/examples/consumer/main.rs b/ipc_test/examples/consumer/main.rs
@@ -75,13 +75,13 @@ fn main() {
         // some additional "work":
         //std::thread::sleep(Duration::from_micros(1));
 
-        ssa.free_idx(slot_info.slot_idx);
+        ssa.free_idx(slot_info.slot_idx).unwrap();
 
         sum += sum_part.0 as f64;
         bytes_processed += SLOT_SIZE_BYTES;
 
         if t0.elapsed() > Duration::from_secs(1) {
-            let slots_free = ssa.num_slots_free();
+            let slots_free = ssa.num_slots_free().unwrap();
             println!(
                 "idx: {idx:5}, sum: {sum_part}, throughput: {:7.2} MiB/s, slots free: {slots_free}",
                 bytes_processed as f32 / 1024.0 / 1024.0

diff --git a/ipc_test/examples/producer/main.rs b/ipc_test/examples/producer/main.rs
@@ -70,7 +70,7 @@ fn handle_connection(
     }
 
     println!("done sending {} items", send_num_items);
-    while ssa.num_slots_free() < ssa.num_slots_total() {
+    while ssa.num_slots_free().unwrap() < ssa.num_slots_total() {
         thread::sleep(Duration::from_millis(100));
     }
     println!("done!")

diff --git a/ipc_test/src/backend_shm.rs b/ipc_test/src/backend_shm.rs
@@ -111,7 +111,7 @@ impl SharedMemory {
 impl Drop for SharedMemory {
     fn drop(&mut self) {
         if self.is_owner {
-            remove_file(&self.handle_path).unwrap();
+            let _ = remove_file(&self.handle_path);
         }
     }
 }