Merge pull request #370 from matthiasbeyer/libimagstore/hook-errors-not-aborting

Libimagstore/hook errors not aborting

libimagstore/src/hook/aspect.rs

@@ -1,3 +1,5 @@
+use libimagerror::trace::trace_error;
+
 use store::FileLockEntry;
 use storeid::StoreId;
 use hook::Hook;
@@ -38,34 +40,32 @@ impl Aspect {
 
 impl StoreIdAccessor for Aspect {
     fn access(&self, id: &StoreId) -> HookResult<()> {
-        use crossbeam;
-
         let accessors : Vec<HDA> = self.hooks.iter().map(|h| h.accessor()).collect();
         if !accessors.iter().all(|a| is_match!(*a, HDA::StoreIdAccess(_))) {
             return Err(HE::new(HEK::AccessTypeViolation, None));
         }
 
-        let threads : Vec<HookResult<()>> = accessors
+        accessors
             .iter()
-            .map(|accessor| {
+            .fold(Ok(()), |acc, accessor| {
-                crossbeam::scope(|scope| {
+                acc.and_then(|_| {
-                    scope.spawn(|| {
+                    let res = match accessor {
-                        match *accessor {
+                        &HDA::StoreIdAccess(accessor) => accessor.access(id),
-                            HDA::StoreIdAccess(accessor) => accessor.access(id),
                         _ => unreachable!(),
-                        }
+                    };
-                        .map_err(|_| ()) // TODO: We're losing the error cause here
-                    })
-                })
-            })
-            .map(|i| i.join().map_err(|_| HE::new(HEK::HookExecutionError, None)))
-            .collect();
 
-        threads
+                    match res {
-            .into_iter()
+                        Ok(res) => Ok(res),
-            .fold(Ok(()), |acc, elem| {
+                        Err(e) => {
-                acc.and_then(|a| {
+                            if !e.is_aborting() {
-                    elem.map(|_| a).map_err(|_| HE::new(HEK::HookExecutionError, None))
+                                trace_error(&e);
+                                // ignore error if it is not aborting, as we printed it already
+                                Ok(())
+                            } else {
+                                Err(e)
+                            }
+                        }
+                    }
                 })
             })
     }
@@ -83,52 +83,63 @@ impl MutableHookDataAccessor for Aspect {
             return Err(HE::new(HEK::AccessTypeViolation, None));
         }
 
-        for accessor in accessors {
-            match accessor {
-                HDA::MutableAccess(accessor)    => try!(accessor.access_mut(fle)),
-
         // TODO: Naiive implementation.
         // More sophisticated version would check whether there are _chunks_ of
         // NonMutableAccess accessors and execute these chunks in parallel. We do not have
         // performance concerns yet, so this is okay.
-                HDA::NonMutableAccess(accessor) => try!(accessor.access(fle)),
+        accessors.iter().fold(Ok(()), |acc, accessor| {
+            acc.and_then(|_| {
+                let res = match accessor {
+                    &HDA::MutableAccess(ref accessor)    => accessor.access_mut(fle),
+                    &HDA::NonMutableAccess(ref accessor) => accessor.access(fle),
                     _ => unreachable!(),
-            }
+                };
-        }
+
+                match res {
+                    Ok(res) => Ok(res),
+                    Err(e) => {
+                        if !e.is_aborting() {
+                            trace_error(&e);
+                            // ignore error if it is not aborting, as we printed it already
                             Ok(())
+                        } else {
+                            Err(e)
+                        }
+                    }
+                }
+            })
+        })
     }
 }
 
 impl NonMutableHookDataAccessor for Aspect {
     fn access(&self, fle: &FileLockEntry) -> HookResult<()> {
-        use crossbeam;
-
         let accessors : Vec<HDA> = self.hooks.iter().map(|h| h.accessor()).collect();
         if !accessors.iter().all(|a| is_match!(*a, HDA::NonMutableAccess(_))) {
             return Err(HE::new(HEK::AccessTypeViolation, None));
         }
 
-        let threads : Vec<HookResult<()>> = accessors
+        accessors
             .iter()
-            .map(|accessor| {
+            .fold(Ok(()), |acc, accessor| {
-                crossbeam::scope(|scope| {
+                acc.and_then(|_| {
-                    scope.spawn(|| {
+                    let res = match accessor {
-                        match *accessor {
+                        &HDA::NonMutableAccess(accessor) => accessor.access(fle),
-                            HDA::NonMutableAccess(accessor) => accessor.access(fle),
                         _ => unreachable!(),
-                        }
+                    };
-                        .map_err(|_| ()) // TODO: We're losing the error cause here
-                    })
-                })
-            })
-            .map(|i| i.join().map_err(|_| HE::new(HEK::HookExecutionError, None)))
-            .collect();
 
-        threads
+                    match res {
-            .into_iter()
+                        Ok(res) => Ok(res),
-            .fold(Ok(()), |acc, elem| {
+                        Err(e) => {
-                acc.and_then(|a| {
+                            if !e.is_aborting() {
-                    elem.map(|_| a).map_err(|_| HE::new(HEK::HookExecutionError, None))
+                                trace_error(&e);
+                                // ignore error if it is not aborting, as we printed it already
+                                Ok(())
+                            } else {
+                                Err(e)
+                            }
+                        }
+                    }
                 })
             })
     }

libimagstore/src/hook/error.rs

@@ -1,10 +1,23 @@
-generate_error_module!(
+use std::convert::Into;
-    generate_error_types!(HookError, HookErrorKind,
+generate_error_imports!();
+
+generate_custom_error_types!(HookError, HookErrorKind, CustomData,
     HookExecutionError  => "Hook exec error",
     AccessTypeViolation => "Hook access type violation"
 );
-);
 
-pub use self::error::HookError;
+#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Copy)]
-pub use self::error::HookErrorKind;
+pub struct CustomData {
+    aborting: bool,
+}
 
+impl HookError {
+
+    pub fn is_aborting(&self) -> bool {
+        match self.custom_data {
+            Some(b) => b.aborting,
+            None => true
+        }
+    }
+
+}

libimagstore/src/store.rs

@@ -28,10 +28,13 @@ use storeid::{IntoStoreId, StoreId, StoreIdIterator};
 use lazyfile::LazyFile;
 
 use hook::aspect::Aspect;
+use hook::error::HookErrorKind;
+use hook::result::HookResult;
 use hook::accessor::{ MutableHookDataAccessor,
             StoreIdAccessor};
 use hook::position::HookPosition;
 use hook::Hook;
+use libimagerror::trace::trace_error;
 
 use libimagerror::into::IntoError;
 
@@ -301,7 +304,13 @@ impl Store {
     pub fn create<'a, S: IntoStoreId>(&'a self, id: S) -> Result<FileLockEntry<'a>> {
         let id = self.storify_id(id.into_storeid());
         if let Err(e) = self.execute_hooks_for_id(self.pre_create_aspects.clone(), &id) {
-            return Err(e);
+            if e.is_aborting() {
+                return Err(e)
+                    .map_err(Box::new)
+                    .map_err(|e| SEK::PreHookExecuteError.into_error_with_cause(e))
+            } else {
+                trace_error(&e);
+            }
         }
 
         let mut hsmap = match self.entries.write() {
@@ -332,7 +341,13 @@ impl Store {
     pub fn retrieve<'a, S: IntoStoreId>(&'a self, id: S) -> Result<FileLockEntry<'a>> {
         let id = self.storify_id(id.into_storeid());
         if let Err(e) = self.execute_hooks_for_id(self.pre_retrieve_aspects.clone(), &id) {
-            return Err(e);
+            if e.is_aborting() {
+                return Err(e)
+                    .map_err(Box::new)
+                    .map_err(|e| SEK::PreHookExecuteError.into_error_with_cause(e))
+            } else {
+                trace_error(&e);
+            }
         }
 
         self.entries
@@ -426,7 +441,13 @@ impl Store {
     /// Return the `FileLockEntry` and write to disk
     pub fn update<'a>(&'a self, mut entry: FileLockEntry<'a>) -> Result<()> {
         if let Err(e) = self.execute_hooks_for_mut_file(self.pre_update_aspects.clone(), &mut entry) {
-            return Err(e);
+            if e.is_aborting() {
+                return Err(e)
+                    .map_err(Box::new)
+                    .map_err(|e| SEK::PreHookExecuteError.into_error_with_cause(e))
+            } else {
+                trace_error(&e);
+            }
         }
 
         if let Err(e) = self._update(&entry) {
@@ -434,6 +455,7 @@ impl Store {
         }
 
         self.execute_hooks_for_mut_file(self.post_update_aspects.clone(), &mut entry)
+            .map_err(|e| SE::new(SEK::PreHookExecuteError, Some(Box::new(e))))
     }
 
     /// Internal method to write to the filesystem store.
@@ -482,7 +504,13 @@ impl Store {
     pub fn delete<S: IntoStoreId>(&self, id: S) -> Result<()> {
         let id = self.storify_id(id.into_storeid());
         if let Err(e) = self.execute_hooks_for_id(self.pre_delete_aspects.clone(), &id) {
-            return Err(e);
+            if e.is_aborting() {
+                return Err(e).map_err(|e| {
+                    SE::new(SEK::PreHookExecuteError, Some(Box::new(e)))
+                })
+            } else {
+                trace_error(&e);
+            }
         }
 
         let mut entries = match self.entries.write() {
@@ -502,6 +530,7 @@ impl Store {
         }
 
         self.execute_hooks_for_id(self.post_delete_aspects.clone(), &id)
+            .map_err(|e| SE::new(SEK::PreHookExecuteError, Some(Box::new(e))))
     }
 
     /// Gets the path where this store is on the disk
@@ -566,29 +595,29 @@ impl Store {
     fn execute_hooks_for_id(&self,
                             aspects: Arc<Mutex<Vec<Aspect>>>,
                             id: &StoreId)
-        -> Result<()>
+        -> HookResult<()>
     {
         match aspects.lock() {
-            Err(_) => return Err(SE::new(SEK::PreHookExecuteError, None)),
+            Err(_) => return Err(HookErrorKind::HookExecutionError.into()),
             Ok(g) => g
         }.iter().fold(Ok(()), |acc, aspect| {
             debug!("[Aspect][exec]: {:?}", aspect);
             acc.and_then(|_| (aspect as &StoreIdAccessor).access(id))
-        }).map_err(|e| SE::new(SEK::PreHookExecuteError, Some(Box::new(e))))
+        }).map_err(Box::new).map_err(|e| HookErrorKind::HookExecutionError.into_error_with_cause(e))
     }
 
     fn execute_hooks_for_mut_file(&self,
                                   aspects: Arc<Mutex<Vec<Aspect>>>,
                                   fle: &mut FileLockEntry)
-        -> Result<()>
+        -> HookResult<()>
     {
         match aspects.lock() {
-            Err(_) => return Err(SE::new(SEK::PreHookExecuteError, None)),
+            Err(_) => return Err(HookErrorKind::HookExecutionError.into()),
             Ok(g) => g
         }.iter().fold(Ok(()), |acc, aspect| {
             debug!("[Aspect][exec]: {:?}", aspect);
             acc.and_then(|_| aspect.access_mut(fle))
-        }).map_err(|e| SE::new(SEK::PreHookExecuteError, Some(Box::new(e))))
+        }).map_err(Box::new).map_err(|e| HookErrorKind::HookExecutionError.into_error_with_cause(e))
     }
 
 }