Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Xintao <[email protected]>
  • Loading branch information
@hunterlxt
hunterlxt committed Jun 2, 2020
1 parent fe2791d commit 829626d
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 70 deletions.
106 changes: 52 additions & 54 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,11 @@
//! }
//! ```
//!
//! It should be noted that the local registry will inherit the global registry when
//! it is created, which means that the inherited part can be shared. When you remove
//! a global fail point action from the local registry, it will affect all threads
//! using this fail point.
//! It should be noted that the local registry will will overwrite the global registry
//! if you register the current thread here. This means that the current thread can only
//! use the fail points configuration of the local registry after registration.
//!
//! Here's a example to show the inheritance process：
//! Here's a example to show the process：
//!
//! ```rust
//! fail::setup();
Expand All @@ -138,10 +137,10 @@
//! FAILPOINTS=p0=return cargo run --features fail/failpoints
//! Finished dev [unoptimized + debuginfo] target(s) in 0.01s
//! Running `target/debug/failpointtest`
//! Global registry: [("p1", "sleep(100)"), ("p0", "return")]
//! Local registry: [("p1", "sleep(100)"), ("p0", "pause")]
//! Global registry: [("p1", "sleep(100)")]
//! Local registry: [("p0", "pause")]
//! Local registry: []
//! Global registry: [("p1", "sleep(100)"), ("p0", "return")]
//! Global registry: [("p1", "sleep(100)")]
//! ```
//!
//! In this example, program update global registry with environment variable first.
Expand Down Expand Up @@ -325,19 +324,6 @@ struct Action {
count: Option<AtomicUsize>,
}

impl Clone for Action {
fn clone(&self) -> Self {
Action {
count: self
.count
.as_ref()
.map(|c| AtomicUsize::new(c.load(Ordering::Relaxed))),
task: self.task.clone(),
freq: self.freq,
}
}
}

impl PartialEq for Action {
fn eq(&self, hs: &Action) -> bool {
if self.task != hs.task || self.freq != hs.freq {
Expand Down Expand Up @@ -477,16 +463,6 @@ struct FailPoint {
actions_str: RwLock<String>,
}

impl Clone for FailPoint {
fn clone(&self) -> Self {
FailPoint {
actions: RwLock::new(self.actions.read().unwrap().clone()),
actions_str: RwLock::new(self.actions_str.read().unwrap().clone()),
..Default::default()
}
}
}

#[cfg_attr(feature = "cargo-clippy", allow(clippy::mutex_atomic))]
impl FailPoint {
fn new() -> FailPoint {
Expand Down Expand Up @@ -576,24 +552,25 @@ pub struct FailPointRegistry {
/// Each thread should be bound to exact one registry. Threads bound to the
/// same registry share the same failpoints configuration.
pub fn new_fail_group() -> FailPointRegistry {
let registry = REGISTRY_GLOBAL.registry.read().unwrap();
let mut new_registry = Registry::new();
for (name, failpoint) in registry.iter() {
new_registry.insert(name.clone(), Arc::new(FailPoint::clone(failpoint)));
}
FailPointRegistry {
registry: Arc::new(RwLock::new(new_registry)),
registry: Arc::new(RwLock::new(Registry::new())),
}
}

impl FailPointRegistry {
/// Register the current thread to this failpoints registry.
pub fn register_current(&self) {
pub fn register_current(&self) -> Result<(), String> {
let id = thread::current().id();
REGISTRY_GROUP
let ret = REGISTRY_GROUP
.write()
.unwrap()
.insert(id, self.registry.clone());

if ret.is_some() {
Err("current thread has been registered with one registry".to_owned())
} else {
Ok(())
}
}

/// Deregister the current thread to this failpoints registry.
Expand Down Expand Up @@ -696,14 +673,13 @@ pub const fn has_failpoints() -> bool {
///
/// Return a vector of `(name, actions)` pairs.
pub fn list() -> Vec<(String, String)> {
let id = thread::current().id();
let group = REGISTRY_GROUP.read().unwrap();
let registry = {
let group = REGISTRY_GROUP.read().unwrap();
let id = thread::current().id();
group.get(&id).unwrap_or(&REGISTRY_GLOBAL.registry).clone()
};

let registry = group
.get(&id)
.unwrap_or(&REGISTRY_GLOBAL.registry)
.read()
.unwrap();
let registry = registry.read().unwrap();

registry
.iter()
Expand All @@ -713,15 +689,15 @@ pub fn list() -> Vec<(String, String)> {

#[doc(hidden)]
pub fn eval<R, F: FnOnce(Option<String>) -> R>(name: &str, f: F) -> Option<R> {
let id = thread::current().id();

let p = {
let group = REGISTRY_GROUP.read().unwrap();
let registry = group
.get(&id)
.unwrap_or(&REGISTRY_GLOBAL.registry)
.read()
.unwrap();
let registry = {
let group = REGISTRY_GROUP.read().unwrap();
let id = thread::current().id();
group.get(&id).unwrap_or(&REGISTRY_GLOBAL.registry).clone()
};

let registry = registry.read().unwrap();

match registry.get(name) {
None => return None,
Some(p) => p.clone(),
Expand Down Expand Up @@ -1125,6 +1101,28 @@ mod tests {
"setup_and_teardown1=return;setup_and_teardown2=pause;",
);
setup();

let group = new_fail_group();
let handler = thread::spawn(move || {
group.register_current().unwrap();
cfg("setup_and_teardown1", "panic").unwrap();
cfg("setup_and_teardown2", "panic").unwrap();
let l = list();
assert!(
l.iter()
.find(|&x| x == &("setup_and_teardown1".to_owned(), "panic".to_owned()))
.is_some()
&& l.iter()
.find(|&x| x == &("setup_and_teardown2".to_owned(), "panic".to_owned()))
.is_some()
&& l.len() == 2
);
remove("setup_and_teardown2");
let l = list();
assert!(l.len() == 1);
});
handler.join().unwrap();

assert_eq!(f1(), 1);

let (tx, rx) = mpsc::channel();
Expand Down
32 changes: 16 additions & 16 deletions tests/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use fail::fail_point;
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_pause() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();
let f = || {
fail_point!("pause");
};
Expand All @@ -21,7 +21,7 @@ fn test_pause() {
let (tx, rx) = mpsc::channel();
let thread_registry = local_registry.clone();
thread::spawn(move || {
thread_registry.register_current();
thread_registry.register_current().unwrap();
// pause
tx.send(f()).unwrap();
// woken up by new order pause, and then pause again.
Expand All @@ -44,7 +44,7 @@ fn test_pause() {
#[test]
fn test_off() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("off", |_| 2);
Expand All @@ -60,7 +60,7 @@ fn test_off() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_return() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("return", |s: Option<String>| s
Expand All @@ -80,7 +80,7 @@ fn test_return() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_sleep() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("sleep");
Expand All @@ -100,7 +100,7 @@ fn test_sleep() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_panic() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("panic");
Expand All @@ -113,7 +113,7 @@ fn test_panic() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_print() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

struct LogCollector(Arc<Mutex<Vec<String>>>);
impl log::Log for LogCollector {
Expand Down Expand Up @@ -149,7 +149,7 @@ fn test_print() {
#[test]
fn test_yield() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("yield");
Expand All @@ -162,7 +162,7 @@ fn test_yield() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_callback() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f1 = || {
fail_point!("cb");
Expand All @@ -186,7 +186,7 @@ fn test_callback() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_delay() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || fail_point!("delay");
let timer = Instant::now();
Expand All @@ -199,7 +199,7 @@ fn test_delay() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_freq_and_count() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = || {
fail_point!("freq_and_count", |s: Option<String>| s
Expand All @@ -223,7 +223,7 @@ fn test_freq_and_count() {
#[cfg_attr(not(feature = "failpoints"), ignore)]
fn test_condition() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let f = |_enabled| {
fail_point!("condition", _enabled, |_| 2);
Expand All @@ -240,7 +240,7 @@ fn test_condition() {
#[test]
fn test_list() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

assert!(!fail::list().contains(&("list".to_string(), "off".to_string())));
fail::cfg("list", "off").unwrap();
Expand All @@ -252,11 +252,11 @@ fn test_list() {
#[test]
fn test_multiple_threads_cleanup() {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();

let (tx, rx) = mpsc::channel();
thread::spawn(move || {
local_registry.register_current();
local_registry.register_current().unwrap();
fail::cfg("thread_point", "sleep(10)").unwrap();
tx.send(()).unwrap();
});
Expand All @@ -272,7 +272,7 @@ fn test_multiple_threads_cleanup() {
let (tx, rx) = mpsc::channel();
let t = thread::spawn(move || {
let local_registry = fail::new_fail_group();
local_registry.register_current();
local_registry.register_current().unwrap();
fail::cfg("thread_point", "panic").unwrap();
let l = fail::list();
assert!(
Expand Down

0 comments on commit 829626d

Please sign in to comment.