From a07e03a25cada785ed1f013a193b836c463c2b82 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 08:54:43 +0200 Subject: [PATCH 1/5] Fix: Renaming entries should also remove the old entry This is a bugfix for a bug in Store::move_entry_by_id(): The old entry was not removed after the new one was created. The bug is a bit subtle. The issue was, that the internal cache held open a reference to the old entry (as StoreEntry object) and when that object got drop()ed, the contents of the entry were written to disk, which resulted in the old entry being recreated. This patch rewrites the function to behave well. The most critical part is that we do not check whether the old entry is borrowed with `hsmap.get()` but rather `hsmap.remove()`. The trick here is that the `StoreEntry` object is dropped in the moment the check is done, clearing the cached object and flushing it to the backend (the disk). After that, we continue doing the filesystem operation and the cache is clean. Signed-off-by: Matthias Beyer --- lib/core/libimagstore/src/store.rs | 32 +++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index cb63162a..5e029e48 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -590,18 +590,21 @@ impl Store { } debug!("New id does not exist in cache"); - // if we do not have an entry here, we fail in `FileAbstraction::rename()` below. - // if we have one, but it is borrowed, we really should not rename it, as this might - // lead to strange errors - if hsmap.get(&old_id).map(|e| e.is_borrowed()).unwrap_or(false) { - return Err(format_err!("Entry already borrowed: {}", old_id)); - } - - debug!("Old id is not yet borrowed"); - let old_id_pb = old_id.clone().with_base(self.path()).into_pathbuf()?; let new_id_pb = new_id.clone().with_base(self.path()).into_pathbuf()?; + if !self.backend.exists(&old_id_pb)? { + return Err(format_err!("Entry does not exist: {}", old_id)); + } + + // if it is borrowed, we really should not rename it, as this might + // lead to strange errors + // + // Also, remove this object from the cache + if hsmap.remove(&old_id).map(|e| e.is_borrowed()).unwrap_or(false) { + return Err(format_err!("Entry already borrowed: {}", old_id)); + } + if self.backend.exists(&new_id_pb)? { return Err(format_err!("Entry already exists: {}", new_id)); } @@ -617,17 +620,6 @@ impl Store { })?; debug!("Rename worked on filesystem"); - - // assert enforced through check hsmap.contains_key(&new_id) above. - // Should therefor never fail - let hsmap_does_not_have_key = hsmap - .remove(&old_id) - .and_then(|mut entry| { - entry.id = new_id.clone(); - hsmap.insert(new_id.clone(), entry) - }) - .is_none(); - assert!(hsmap_does_not_have_key); } debug!("Moved"); From d1078590c79e8ff2b836c08acfa9226118cc7975 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 09:46:39 +0200 Subject: [PATCH 2/5] Fix: Testing backend bug where an entry was not properly rewritten When moving an entry, what we did is copying the entry inside the backend abstraction (the hashmap) from one key to another. But as the entry itself does also encode its location, we actually have to rewrite the entire entry. This patch fixes this. Signed-off-by: Matthias Beyer --- .../libimagstore/src/file_abstraction/inmemory.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/core/libimagstore/src/file_abstraction/inmemory.rs b/lib/core/libimagstore/src/file_abstraction/inmemory.rs index d8c7980b..9aa677b1 100644 --- a/lib/core/libimagstore/src/file_abstraction/inmemory.rs +++ b/lib/core/libimagstore/src/file_abstraction/inmemory.rs @@ -28,6 +28,7 @@ use libimagerror::errors::ErrorMsg as EM; use failure::Fallible as Result; use failure::Error; +use failure::err_msg; use super::FileAbstraction; use super::FileAbstractionInstance; @@ -137,7 +138,18 @@ impl FileAbstraction for InMemoryFileAbstraction { let backend = mtx.get_mut(); let a = backend.remove(from).ok_or_else(|| EM::FileNotFound)?; - backend.insert(to.clone(), a); + let new_entry = { + let new_location = if to.starts_with("/") { + let s = to.to_str().map(String::from).ok_or_else(|| err_msg("Failed to convert path to str"))?; + PathBuf::from(s.replace("/", "")) + } else { + to.to_path_buf() + }; + + Entry::from_str(crate::storeid::StoreId::new(new_location)?, &a.to_str()?)? + }; + + backend.insert(to.clone(), new_entry); debug!("Renaming: {:?} -> {:?} worked", from, to); Ok(()) } From 583f9727880e81b1827cfb33dbbd924589eda33c Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 09:48:50 +0200 Subject: [PATCH 3/5] Add test: Test moving entry with writing to it before and after moving Signed-off-by: Matthias Beyer --- lib/core/libimagstore/src/store.rs | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 5e029e48..1e195fb0 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -1226,4 +1226,43 @@ mod store_tests { } } + #[test] + fn test_moving_entry_with_writing_before_and_after() { + use crate::storeid::StoreId; + setup_logging(); + + let store = get_store(); + + let old_name = StoreId::new(PathBuf::from("old")).unwrap(); + let new_name = StoreId::new(PathBuf::from("new")).unwrap(); + + debug!("Creating old entry"); + { + let mut entry = store.create(old_name.clone()).unwrap(); + entry.get_content_mut().push_str("first line"); + drop(entry); + } // make sure entry is dropped + + debug!("Moving"); + store.move_by_id(old_name.clone(), new_name.clone()).unwrap(); + + debug!("Getting old entry again (should not work)"); + assert!(store.get(old_name).unwrap().is_none()); + + debug!("Getting new entry"); + { + let mut entry = store.get(new_name.clone()).unwrap().unwrap(); + assert_eq!(entry.get_content(), "first line"); + entry.get_content_mut().push_str("second line"); + drop(entry); + } // make sure entry is dropped + + { + debug!("Getting new entry again"); + debug!("Store = {:#?}", store); + let new_entry = store.get(new_name).unwrap().unwrap(); + assert_eq!(new_entry.get_content(), "first linesecond line"); + } + } + } From 7348378a965d25ec0ef8e9f698ad33306031a5e3 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 09:53:38 +0200 Subject: [PATCH 4/5] Fix test: Check whether in cache, then get, then check again After moving an entry, the entry should _not_ be in the cache. This is just a decision that has to be made, whether we cache the moved entry or not. I decided to not cache because it is results in less code. After that check, we get the entry from the backend and then we can check whether the entry is in the cache, which is then should be. Signed-off-by: Matthias Beyer --- lib/core/libimagstore/src/store.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 1e195fb0..24b57052 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -1212,7 +1212,9 @@ mod store_tests { assert!(r.map_err(|e| debug!("ERROR: {:?}", e)).is_ok()); { - assert!(store.entries.read().unwrap().get(&id_mv).is_some()); + assert!(store.entries.read().unwrap().get(&id_mv).is_none()); // entry not in cache yet + assert!(store.get(id_mv.clone()).unwrap().is_some()); // get entry from backend + assert!(store.entries.read().unwrap().get(&id_mv).is_some()); // entry in cache } let res = store.get(id.clone()); From 2a407d161b905bc7da52f9bb0e4804ae3f07e53e Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 09:54:54 +0200 Subject: [PATCH 5/5] Add some debug output Signed-off-by: Matthias Beyer --- lib/core/libimagstore/src/store.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 24b57052..49f76164 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -1196,22 +1196,29 @@ mod store_tests { let id = StoreId::new(PathBuf::from(format!("t-{}", n))).unwrap(); let id_mv = StoreId::new(PathBuf::from(format!("t-{}", n - 1))).unwrap(); + debug!("Trying to move: {} -> {}", id, id_mv); + { + debug!("Checking presence: {}", id); assert!(store.entries.read().unwrap().get(&id).is_none()); } { + debug!("Creating : {}", id); assert!(store.create(id.clone()).is_ok()); } { + debug!("Checking presence: {}", id); assert!(store.entries.read().unwrap().get(&id).is_some()); } + debug!("Moving: {} -> {}", id, id_mv); let r = store.move_by_id(id.clone(), id_mv.clone()); assert!(r.map_err(|e| debug!("ERROR: {:?}", e)).is_ok()); { + debug!("Checking presence: {}", id_mv); assert!(store.entries.read().unwrap().get(&id_mv).is_none()); // entry not in cache yet assert!(store.get(id_mv.clone()).unwrap().is_some()); // get entry from backend assert!(store.entries.read().unwrap().get(&id_mv).is_some()); // entry in cache