From 7211df6719b756bd85c0390ec7805ef29dc4c0f7 Mon Sep 17 00:00:00 2001 From: Paul Masurel Date: Mon, 22 Jul 2019 13:17:21 +0900 Subject: [PATCH] Failrs (#600) * Single thread tests * Isolating fail tests into a different binary --- Cargo.toml | 16 +++++++ appveyor.yml | 2 +- run-tests.sh | 2 +- src/directory/managed_directory.rs | 44 ++++++++++---------- src/directory/mmap_directory.rs | 1 - src/directory/mod.rs | 2 +- src/directory/ram_directory.rs | 5 +++ src/indexer/index_writer.rs | 28 +------------ tests/failpoints/mod.rs | 67 ++++++++++++++++++++++++++++++ tests/mod.rs | 1 + 10 files changed, 114 insertions(+), 54 deletions(-) create mode 100644 tests/failpoints/mod.rs create mode 100644 tests/mod.rs diff --git a/Cargo.toml b/Cargo.toml index c1dc69ee6..7fc6a5877 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -83,3 +83,19 @@ wasm-bindgen = ["uuid/wasm-bindgen"] [badges] travis-ci = { repository = "tantivy-search/tantivy" } + +[dev-dependencies.fail] +features = ["failpoints"] + + +# Following the "fail" crate best practises, we isolate +# tests that define specific behavior in fail check points +# in a different binary. +# +# We do that because, fail rely on a global definition of +# failpoints behavior and hence, it is incompatible with +# multithreading. +[[test]] +name = "failpoints" +path = "tests/failpoints/mod.rs" +required-features = ["fail/failpoints"] \ No newline at end of file diff --git a/appveyor.yml b/appveyor.yml index 685b04d3a..7f9035e03 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -18,5 +18,5 @@ install: build: false test_script: - - REM SET RUST_LOG=tantivy,test & cargo test --verbose --no-default-features --features mmap -- --test-threads 1 + - REM SET RUST_LOG=tantivy,test & cargo test --verbose --no-default-features --features mmap - REM SET RUST_BACKTRACE=1 & cargo build --examples diff --git a/run-tests.sh b/run-tests.sh index 8f3f934d5..c179d0f6d 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -1,2 +1,2 @@ #!/bin/bash -cargo test --features "failpoints" +cargo test diff --git a/src/directory/managed_directory.rs b/src/directory/managed_directory.rs index cb43f7ac0..ea914a5f6 100644 --- a/src/directory/managed_directory.rs +++ b/src/directory/managed_directory.rs @@ -135,28 +135,28 @@ impl ManagedDirectory { files_to_delete.push(managed_path.clone()); } } + } else { + error!("Failed to acquire lock for GC"); } } let mut deleted_files = vec![]; - { - for file_to_delete in files_to_delete { - match self.delete(&file_to_delete) { - Ok(_) => { - info!("Deleted {:?}", file_to_delete); - deleted_files.push(file_to_delete); - } - Err(file_error) => { - match file_error { - DeleteError::FileDoesNotExist(_) => { - deleted_files.push(file_to_delete); - } - DeleteError::IOError(_) => { - if !cfg!(target_os = "windows") { - // On windows, delete is expected to fail if the file - // is mmapped. - error!("Failed to delete {:?}", file_to_delete); - } + for file_to_delete in files_to_delete { + match self.delete(&file_to_delete) { + Ok(_) => { + info!("Deleted {:?}", file_to_delete); + deleted_files.push(file_to_delete); + } + Err(file_error) => { + match file_error { + DeleteError::FileDoesNotExist(_) => { + deleted_files.push(file_to_delete); + } + DeleteError::IOError(_) => { + if !cfg!(target_os = "windows") { + // On windows, delete is expected to fail if the file + // is mmapped. + error!("Failed to delete {:?}", file_to_delete); } } } @@ -171,11 +171,9 @@ impl ManagedDirectory { .meta_informations .write() .expect("Managed directory wlock poisoned (2)."); - { - let managed_paths_write = &mut meta_informations_wlock.managed_paths; - for delete_file in &deleted_files { - managed_paths_write.remove(delete_file); - } + let managed_paths_write = &mut meta_informations_wlock.managed_paths; + for delete_file in &deleted_files { + managed_paths_write.remove(delete_file); } if save_managed_paths(self.directory.as_mut(), &meta_informations_wlock).is_err() { error!("Failed to save the list of managed files."); diff --git a/src/directory/mmap_directory.rs b/src/directory/mmap_directory.rs index d73b9c160..e19b43057 100644 --- a/src/directory/mmap_directory.rs +++ b/src/directory/mmap_directory.rs @@ -417,7 +417,6 @@ impl Directory for MmapDirectory { /// Any entry associated to the path in the mmap will be /// removed before the file is deleted. fn delete(&self, path: &Path) -> result::Result<(), DeleteError> { - debug!("Deleting file {:?}", path); let full_path = self.resolve_path(path); match fs::remove_file(&full_path) { Ok(_) => self diff --git a/src/directory/mod.rs b/src/directory/mod.rs index 1b6f633a1..70fa01348 100644 --- a/src/directory/mod.rs +++ b/src/directory/mod.rs @@ -29,7 +29,7 @@ use std::io::{BufWriter, Write}; #[cfg(feature = "mmap")] pub use self::mmap_directory::MmapDirectory; -pub(crate) use self::managed_directory::ManagedDirectory; +pub use self::managed_directory::ManagedDirectory; /// Write object for Directory. /// diff --git a/src/directory/ram_directory.rs b/src/directory/ram_directory.rs index 0ddf50f53..53e7715f8 100644 --- a/src/directory/ram_directory.rs +++ b/src/directory/ram_directory.rs @@ -145,6 +145,11 @@ impl Directory for RAMDirectory { } fn delete(&self, path: &Path) -> result::Result<(), DeleteError> { + fail_point!("RAMDirectory::delete", |_| { + use crate::directory::error::IOError; + let io_error = IOError::from(io::Error::from(io::ErrorKind::Other)); + Err(DeleteError::from(io_error)) + }); self.fs.write().unwrap().delete(path) } diff --git a/src/indexer/index_writer.rs b/src/indexer/index_writer.rs index c52e1f44d..fa5ae0b5e 100644 --- a/src/indexer/index_writer.rs +++ b/src/indexer/index_writer.rs @@ -761,6 +761,7 @@ mod tests { use crate::Index; use crate::ReloadPolicy; use crate::Term; + use fail; #[test] fn test_operations_group() { @@ -933,7 +934,6 @@ mod tests { #[test] fn test_with_merges() { - let _guard = fail::FailScenario::setup(); let mut schema_builder = schema::Schema::builder(); let text_field = schema_builder.add_text_field("text", schema::TEXT); let index = Index::create_in_ram(schema_builder.build()); @@ -1044,32 +1044,6 @@ mod tests { assert_eq!(num_docs_containing("b"), 100); } - #[cfg(feature = "failpoints")] - #[test] - fn test_write_commit_fails() { - let _guard = fail::FailScenario::setup(); - let mut schema_builder = schema::Schema::builder(); - let text_field = schema_builder.add_text_field("text", schema::TEXT); - let index = Index::create_in_ram(schema_builder.build()); - - let mut index_writer = index.writer_with_num_threads(1, 3_000_000).unwrap(); - for _ in 0..100 { - index_writer.add_document(doc!(text_field => "a")); - } - index_writer.commit().unwrap(); - fail::cfg("RAMDirectory::atomic_write", "return(error_write_failed)").unwrap(); - for _ in 0..100 { - index_writer.add_document(doc!(text_field => "b")); - } - assert!(index_writer.commit().is_err()); - let num_docs_containing = |s: &str| { - let term_a = Term::from_field_text(text_field, s); - index.reader().unwrap().searcher().doc_freq(&term_a) - }; - assert_eq!(num_docs_containing("a"), 100); - assert_eq!(num_docs_containing("b"), 0); - } - #[test] fn test_add_then_delete_all_documents() { let mut schema_builder = schema::Schema::builder(); diff --git a/tests/failpoints/mod.rs b/tests/failpoints/mod.rs new file mode 100644 index 000000000..f8360b36f --- /dev/null +++ b/tests/failpoints/mod.rs @@ -0,0 +1,67 @@ +use fail; +use std::io::Write; +use std::path::Path; +use tantivy::directory::{Directory, ManagedDirectory, RAMDirectory}; +use tantivy::doc; +use tantivy::schema::{Schema, TEXT}; +use tantivy::{Index, Term}; + +#[test] +fn test_failpoints_managed_directory_gc_if_delete_fails() { + let scenario = fail::FailScenario::setup(); + + let test_path: &'static Path = Path::new("some_path_for_test"); + + let ram_directory = RAMDirectory::create(); + let mut managed_directory = ManagedDirectory::wrap(ram_directory).unwrap(); + managed_directory + .open_write(test_path) + .unwrap() + .flush() + .unwrap(); + assert!(managed_directory.exists(test_path)); + // triggering gc and setting the delete operation to fail. + // + // We are checking that the gc operation is not removing the + // file from managed.json to ensure that the file will be removed + // in the next gc. + // + // The initial 1*off is there to allow for the removal of the + // lock file. + fail::cfg("RAMDirectory::delete", "1*off->1*return").unwrap(); + managed_directory.garbage_collect(Default::default); + assert!(managed_directory.exists(test_path)); + + // running the gc a second time should remove the file. + managed_directory.garbage_collect(Default::default); + assert!( + !managed_directory.exists(test_path), + "The file should have been deleted" + ); +} + +#[test] +fn test_write_commit_fails() { + let _fail_scenario_guard = fail::FailScenario::setup(); + let mut schema_builder = Schema::builder(); + let text_field = schema_builder.add_text_field("text", TEXT); + let index = Index::create_in_ram(schema_builder.build()); + + let mut index_writer = index.writer_with_num_threads(1, 3_000_000).unwrap(); + for _ in 0..100 { + index_writer.add_document(doc!(text_field => "a")); + } + index_writer.commit().unwrap(); + fail::cfg("RAMDirectory::atomic_write", "return(error_write_failed)").unwrap(); + for _ in 0..100 { + index_writer.add_document(doc!(text_field => "b")); + } + assert!(index_writer.commit().is_err()); + + let num_docs_containing = |s: &str| { + let term_a = Term::from_field_text(text_field, s); + index.reader().unwrap().searcher().doc_freq(&term_a) + }; + assert_eq!(num_docs_containing("a"), 100); + assert_eq!(num_docs_containing("b"), 0); +} diff --git a/tests/mod.rs b/tests/mod.rs new file mode 100644 index 000000000..0ea8d38bf --- /dev/null +++ b/tests/mod.rs @@ -0,0 +1 @@ +mod failpoints;