From ae66b00f5f7f44ac06c13b14a5a1a42a1953497a Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 18:10:37 +0200 Subject: [PATCH 1/4] store tests: Add erroring-hook-tests --- libimagstore/src/store.rs | 86 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 07511e28..919b6f50 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2594,5 +2594,91 @@ aspect = "test" assert!(store.create(pb.clone()).is_ok()); } + + fn get_store_with_aborting_hook_at_pos(pos: HP) -> Store { + let mut store = get_store_with_config(); + let hook = TestHook::new(pos.clone(), false, true); + + assert!(store.register_hook(pos, "test", Box::new(hook)).map_err(|e| println!("{:?}", e)).is_ok()); + store + } + + fn default_test_id() -> StoreId { + StoreId::new_baseless(PathBuf::from("test")).unwrap() + } + + #[test] + fn test_pre_create_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PreCreate); + assert!(store.create(default_test_id()).is_err()); + } + + #[test] + fn test_pre_retrieve_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PreRetrieve); + assert!(store.retrieve(default_test_id()).is_err()); + } + + #[test] + fn test_pre_delete_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PreDelete); + assert!(store.delete(default_test_id()).is_err()); + } + + #[test] + fn test_pre_update_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PreUpdate); + let fle = store.create(default_test_id()).unwrap(); + + assert!(store.update(fle).is_err()); + } + + #[test] + fn test_post_create_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PostCreate); + let pb = default_test_id(); + + assert!(store.create(pb.clone()).is_err()); + + // But the entry exists, as the hook fails post-create + assert!(store.entries.read().unwrap().get(&pb.with_base(store.path().clone())).is_some()); + } + + #[test] + fn test_post_retrieve_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PostRetrieve); + let pb = default_test_id(); + + assert!(store.retrieve(pb.clone()).is_err()); + + // But the entry exists, as the hook fails post-retrieve + assert!(store.entries.read().unwrap().get(&pb.with_base(store.path().clone())).is_some()); + } + + #[test] + fn test_post_delete_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PostDelete); + let pb = default_test_id(); + + assert!(store.create(pb.clone()).is_ok()); + let pb = pb.with_base(store.path().clone()); + assert!(store.entries.read().unwrap().get(&pb).is_some()); + + assert!(store.delete(pb.clone()).is_err()); + // But the entry is removed, as we fail post-delete + assert!(store.entries.read().unwrap().get(&pb).is_none()); + } + + #[test] + fn test_post_update_error() { + let store = get_store_with_aborting_hook_at_pos(HP::PostUpdate); + let pb = default_test_id(); + let fle = store.create(pb.clone()).unwrap(); + let pb = pb.with_base(store.path().clone()); + + assert!(store.entries.read().unwrap().get(&pb).is_some()); + assert!(store.update(fle).is_err()); + } + } From d375a6d2c6f031e7c5b602975038992687040d85 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 18:26:48 +0200 Subject: [PATCH 2/4] Add output to test helper, so we can see in the trace whats happening --- libimagstore/src/store.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 919b6f50..4172b4b2 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2502,12 +2502,16 @@ mod store_hook_tests { } fn get_result(succeed: bool, abort: bool) -> HookResult<()> { + println!("Generting result: succeed = {}, abort = {}", succeed, abort); if succeed { + println!("Generating result: Ok(())"); Ok(()) } else { if abort { + println!("Generating result: Err(_), aborting"); Err(HEK::HookExecutionError.into_error()) } else { + println!("Generating result: Err(_), not aborting"); let custom = CustomData::default().aborting(false); Err(HEK::HookExecutionError.into_error().with_custom_data(custom)) } From 7b11e7dabbcaa61c1250451e91367ceacecc9ed4 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 18:46:50 +0200 Subject: [PATCH 3/4] Add testing implementation for Drop for FileLockEntry --- libimagstore/src/store.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 4172b4b2..a423a059 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -879,6 +879,7 @@ impl<'a> DerefMut for FileLockEntry<'a> { } } +#[cfg(not(test))] impl<'a> Drop for FileLockEntry<'a> { /// This will silently ignore errors, use `Store::update` if you want to catch the errors fn drop(&mut self) { @@ -886,6 +887,15 @@ impl<'a> Drop for FileLockEntry<'a> { } } +#[cfg(test)] +impl<'a> Drop for FileLockEntry<'a> { + /// This will not silently ignore errors but prints the result of the _update() call for testing + fn drop(&mut self) { + println!("Drop Result: {:?}", self.store._update(self)); + } +} + + /// `EntryContent` type pub type EntryContent = String; From 5202c5112a63bcfd4ca77081896f7cce40eab768 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 19:20:34 +0200 Subject: [PATCH 4/4] Add flag for _update() whether precense should be modified This is a bugfix for an very particular issue. Here's what happens: If we create() an FileLockEntry and then update() it, we are running into a problem because update() calls _update() which changes the precense status of a FileLockEntry. Because update() is _consuming_, the FileLockEntry gets drop()ed afterwards. This _again_ causes _update() to be called, but with a new presence status, which is not true in this moment (as the FileLockEntry is still borrowed, but we already marked it as present). This patch is a short-term fix. The real problem is, that our Store interface is consuming. If the Store interface would be non-consuming, this issue wouldn't happen, as the drop() call would not happen. I'm rather sure that this patch will not be reverted in the process of rewriting the Store interface to be non-consuming. But we never know. --- libimagstore/src/store.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index a423a059..f7b3e17f 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -507,7 +507,7 @@ impl Store { .map_err_into(SEK::UpdateCallError); } - if let Err(e) = self._update(&entry) { + if let Err(e) = self._update(&entry, false) { return Err(e).map_err_into(SEK::UpdateCallError); } @@ -522,7 +522,7 @@ impl Store { /// # Assumptions /// This method assumes that entry is dropped _right after_ the call, hence /// it is not public. - fn _update<'a>(&'a self, entry: &FileLockEntry<'a>) -> Result<()> { + fn _update<'a>(&'a self, entry: &FileLockEntry<'a>, modify_presence: bool) -> Result<()> { let mut hsmap = match self.entries.write() { Err(_) => return Err(SE::new(SEK::LockPoisoned, None)), Ok(e) => e, @@ -537,7 +537,9 @@ impl Store { debug!("Writing Entry"); try!(se.write_entry(&entry.entry)); - se.status = StoreEntryStatus::Present; + if modify_presence { + se.status = StoreEntryStatus::Present; + } Ok(()) } @@ -883,7 +885,7 @@ impl<'a> DerefMut for FileLockEntry<'a> { impl<'a> Drop for FileLockEntry<'a> { /// This will silently ignore errors, use `Store::update` if you want to catch the errors fn drop(&mut self) { - let _ = self.store._update(self); + let _ = self.store._update(self, true); } } @@ -891,7 +893,7 @@ impl<'a> Drop for FileLockEntry<'a> { impl<'a> Drop for FileLockEntry<'a> { /// This will not silently ignore errors but prints the result of the _update() call for testing fn drop(&mut self) { - println!("Drop Result: {:?}", self.store._update(self)); + println!("Drop Result: {:?}", self.store._update(self, true)); } }