Skip to content

Commit 8184ded

Browse files
hunterlxthunterlxt
committed
address comments
Signed-off-by: Xintao <[email protected]>
1 parent fe2791d commit 8184ded

File tree

2 files changed

+85
-87
lines changed

2 files changed

+85
-87
lines changed

‎src/lib.rs‎

Lines changed: 55 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@
9999
//! #[test]
100100
//! #[should_panic]
101101
//! fn test_fallible_work() {
102-
//! let local_registry = fail::new_fail_group();
102+
//! let local_registry = fail::create_registry();
103103
//! local_registry.register_current();
104104
//! fail::cfg("read-dir", "panic").unwrap();
105105
//!
@@ -109,19 +109,18 @@
109109
//! }
110110
//! ```
111111
//!
112-
//! It should be noted that the local registry will inherit the global registry when
113-
//! it is created, which means that the inherited part can be shared. When you remove
114-
//! a global fail point action from the local registry, it will affect all threads
115-
//! using this fail point.
112+
//! It should be noted that the local registry will will overwrite the global registry
113+
//! if you register the current thread here. This means that the current thread can only
114+
//! use the fail points configuration of the local registry after registration.
116115
//!
117-
//! Here's a example to show the inheritance process：
116+
//! Here's a example to show the process：
118117
//!
119118
//! ```rust
120119
//! fail::setup();
121120
//! fail::cfg("p1", "sleep(100)").unwrap();
122121
//! println!("Global registry: {:?}", fail::list());
123122
//! {
124-
//! let local_registry = fail::new_fail_group();
123+
//! let local_registry = fail::create_registry();
125124
//! local_registry.register_current();
126125
//! fail::cfg("p0", "pause").unwrap();
127126
//! println!("Local registry: {:?}", fail::list());
@@ -138,10 +137,10 @@
138137
//! FAILPOINTS=p0=return cargo run --features fail/failpoints
139138
//! Finished dev [unoptimized + debuginfo] target(s) in 0.01s
140139
//! Running `target/debug/failpointtest`
141-
//! Global registry: [("p1", "sleep(100)"), ("p0", "return")]
142-
//! Local registry: [("p1", "sleep(100)"), ("p0", "pause")]
140+
//! Global registry: [("p1", "sleep(100)")]
141+
//! Local registry: [("p0", "pause")]
143142
//! Local registry: []
144-
//! Global registry: [("p1", "sleep(100)"), ("p0", "return")]
143+
//! Global registry: [("p1", "sleep(100)")]
145144
//! ```
146145
//!
147146
//! In this example, program update global registry with environment variable first.
@@ -325,19 +324,6 @@ struct Action {
325324
count: Option<AtomicUsize>,
326325
}
327326

328-
impl Clone for Action {
329-
fn clone(&self) -> Self {
330-
Action {
331-
count: self
332-
.count
333-
.as_ref()
334-
.map(|c| AtomicUsize::new(c.load(Ordering::Relaxed))),
335-
task: self.task.clone(),
336-
freq: self.freq,
337-
}
338-
}
339-
}
340-
341327
impl PartialEq for Action {
342328
fn eq(&self, hs: &Action) -> bool {
343329
if self.task != hs.task || self.freq != hs.freq {
@@ -477,16 +463,6 @@ struct FailPoint {
477463
actions_str: RwLock<String>,
478464
}
479465

480-
impl Clone for FailPoint {
481-
fn clone(&self) -> Self {
482-
FailPoint {
483-
actions: RwLock::new(self.actions.read().unwrap().clone()),
484-
actions_str: RwLock::new(self.actions_str.read().unwrap().clone()),
485-
..Default::default()
486-
}
487-
}
488-
}
489-
490466
#[cfg_attr(feature = "cargo-clippy", allow(clippy::mutex_atomic))]
491467
impl FailPoint {
492468
fn new() -> FailPoint {
@@ -575,25 +551,26 @@ pub struct FailPointRegistry {
575551
///
576552
/// Each thread should be bound to exact one registry. Threads bound to the
577553
/// same registry share the same failpoints configuration.
578-
pub fn new_fail_group() -> FailPointRegistry {
579-
let registry = REGISTRY_GLOBAL.registry.read().unwrap();
580-
let mut new_registry = Registry::new();
581-
for (name, failpoint) in registry.iter() {
582-
new_registry.insert(name.clone(), Arc::new(FailPoint::clone(failpoint)));
583-
}
554+
pub fn create_registry() -> FailPointRegistry {
584555
FailPointRegistry {
585-
registry: Arc::new(RwLock::new(new_registry)),
556+
registry: Arc::new(RwLock::new(Registry::new())),
586557
}
587558
}
588559

589560
impl FailPointRegistry {
590561
/// Register the current thread to this failpoints registry.
591-
pub fn register_current(&self) {
562+
pub fn register_current(&self) -> Result<(), String> {
592563
let id = thread::current().id();
593-
REGISTRY_GROUP
564+
let ret = REGISTRY_GROUP
594565
.write()
595566
.unwrap()
596567
.insert(id, self.registry.clone());
568+
569+
if ret.is_some() {
570+
Err("current thread has been registered with one registry".to_owned())
571+
} else {
572+
Ok(())
573+
}
597574
}
598575

599576
/// Deregister the current thread to this failpoints registry.
@@ -696,14 +673,13 @@ pub const fn has_failpoints() -> bool {
696673
///
697674
/// Return a vector of `(name, actions)` pairs.
698675
pub fn list() -> Vec<(String, String)> {
699-
let id = thread::current().id();
700-
let group = REGISTRY_GROUP.read().unwrap();
676+
let registry = {
677+
let group = REGISTRY_GROUP.read().unwrap();
678+
let id = thread::current().id();
679+
group.get(&id).unwrap_or(&REGISTRY_GLOBAL.registry).clone()
680+
};
701681

702-
let registry = group
703-
.get(&id)
704-
.unwrap_or(&REGISTRY_GLOBAL.registry)
705-
.read()
706-
.unwrap();
682+
let registry = registry.read().unwrap();
707683

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

714690
#[doc(hidden)]
715691
pub fn eval<R, F: FnOnce(Option<String>) -> R>(name: &str, f: F) -> Option<R> {
716-
let id = thread::current().id();
717-
718692
let p = {
719-
let group = REGISTRY_GROUP.read().unwrap();
720-
let registry = group
721-
.get(&id)
722-
.unwrap_or(&REGISTRY_GLOBAL.registry)
723-
.read()
724-
.unwrap();
693+
let registry = {
694+
let group = REGISTRY_GROUP.read().unwrap();
695+
let id = thread::current().id();
696+
group.get(&id).unwrap_or(&REGISTRY_GLOBAL.registry).clone()
697+
};
698+
699+
let registry = registry.read().unwrap();
700+
725701
match registry.get(name) {
726702
None => return None,
727703
Some(p) => p.clone(),
@@ -1125,6 +1101,28 @@ mod tests {
11251101
"setup_and_teardown1=return;setup_and_teardown2=pause;",
11261102
);
11271103
setup();
1104+
1105+
let group = create_registry();
1106+
let handler = thread::spawn(move || {
1107+
group.register_current().unwrap();
1108+
cfg("setup_and_teardown1", "panic").unwrap();
1109+
cfg("setup_and_teardown2", "panic").unwrap();
1110+
let l = list();
1111+
assert!(
1112+
l.iter()
1113+
.find(|&x| x == &("setup_and_teardown1".to_owned(), "panic".to_owned()))
1114+
.is_some()
1115+
&& l.iter()
1116+
.find(|&x| x == &("setup_and_teardown2".to_owned(), "panic".to_owned()))
1117+
.is_some()
1118+
&& l.len() == 2
1119+
);
1120+
remove("setup_and_teardown2");
1121+
let l = list();
1122+
assert!(l.len() == 1);
1123+
});
1124+
handler.join().unwrap();
1125+
11281126
assert_eq!(f1(), 1);
11291127

11301128
let (tx, rx) = mpsc::channel();

‎tests/tests.rs‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ use fail::fail_point;
1010
#[test]
1111
#[cfg_attr(not(feature = "failpoints"), ignore)]
1212
fn test_pause() {
13-
let local_registry = fail::new_fail_group();
14-
local_registry.register_current();
13+
let local_registry = fail::create_registry();
14+
local_registry.register_current().unwrap();
1515
let f = || {
1616
fail_point!("pause");
1717
};
@@ -21,7 +21,7 @@ fn test_pause() {
2121
let (tx, rx) = mpsc::channel();
2222
let thread_registry = local_registry.clone();
2323
thread::spawn(move || {
24-
thread_registry.register_current();
24+
thread_registry.register_current().unwrap();
2525
// pause
2626
tx.send(f()).unwrap();
2727
// woken up by new order pause, and then pause again.
@@ -43,8 +43,8 @@ fn test_pause() {
4343

4444
#[test]
4545
fn test_off() {
46-
let local_registry = fail::new_fail_group();
47-
local_registry.register_current();
46+
let local_registry = fail::create_registry();
47+
local_registry.register_current().unwrap();
4848

4949
let f = || {
5050
fail_point!("off", |_| 2);
@@ -59,8 +59,8 @@ fn test_off() {
5959
#[test]
6060
#[cfg_attr(not(feature = "failpoints"), ignore)]
6161
fn test_return() {
62-
let local_registry = fail::new_fail_group();
63-
local_registry.register_current();
62+
let local_registry = fail::create_registry();
63+
local_registry.register_current().unwrap();
6464

6565
let f = || {
6666
fail_point!("return", |s: Option<String>| s
@@ -79,8 +79,8 @@ fn test_return() {
7979
#[test]
8080
#[cfg_attr(not(feature = "failpoints"), ignore)]
8181
fn test_sleep() {
82-
let local_registry = fail::new_fail_group();
83-
local_registry.register_current();
82+
let local_registry = fail::create_registry();
83+
local_registry.register_current().unwrap();
8484

8585
let f = || {
8686
fail_point!("sleep");
@@ -99,8 +99,8 @@ fn test_sleep() {
9999
#[should_panic]
100100
#[cfg_attr(not(feature = "failpoints"), ignore)]
101101
fn test_panic() {
102-
let local_registry = fail::new_fail_group();
103-
local_registry.register_current();
102+
let local_registry = fail::create_registry();
103+
local_registry.register_current().unwrap();
104104

105105
let f = || {
106106
fail_point!("panic");
@@ -112,8 +112,8 @@ fn test_panic() {
112112
#[test]
113113
#[cfg_attr(not(feature = "failpoints"), ignore)]
114114
fn test_print() {
115-
let local_registry = fail::new_fail_group();
116-
local_registry.register_current();
115+
let local_registry = fail::create_registry();
116+
local_registry.register_current().unwrap();
117117

118118
struct LogCollector(Arc<Mutex<Vec<String>>>);
119119
impl log::Log for LogCollector {
@@ -148,8 +148,8 @@ fn test_print() {
148148

149149
#[test]
150150
fn test_yield() {
151-
let local_registry = fail::new_fail_group();
152-
local_registry.register_current();
151+
let local_registry = fail::create_registry();
152+
local_registry.register_current().unwrap();
153153

154154
let f = || {
155155
fail_point!("yield");
@@ -161,8 +161,8 @@ fn test_yield() {
161161
#[test]
162162
#[cfg_attr(not(feature = "failpoints"), ignore)]
163163
fn test_callback() {
164-
let local_registry = fail::new_fail_group();
165-
local_registry.register_current();
164+
let local_registry = fail::create_registry();
165+
local_registry.register_current().unwrap();
166166

167167
let f1 = || {
168168
fail_point!("cb");
@@ -185,8 +185,8 @@ fn test_callback() {
185185
#[test]
186186
#[cfg_attr(not(feature = "failpoints"), ignore)]
187187
fn test_delay() {
188-
let local_registry = fail::new_fail_group();
189-
local_registry.register_current();
188+
let local_registry = fail::create_registry();
189+
local_registry.register_current().unwrap();
190190

191191
let f = || fail_point!("delay");
192192
let timer = Instant::now();
@@ -198,8 +198,8 @@ fn test_delay() {
198198
#[test]
199199
#[cfg_attr(not(feature = "failpoints"), ignore)]
200200
fn test_freq_and_count() {
201-
let local_registry = fail::new_fail_group();
202-
local_registry.register_current();
201+
let local_registry = fail::create_registry();
202+
local_registry.register_current().unwrap();
203203

204204
let f = || {
205205
fail_point!("freq_and_count", |s: Option<String>| s
@@ -222,8 +222,8 @@ fn test_freq_and_count() {
222222
#[test]
223223
#[cfg_attr(not(feature = "failpoints"), ignore)]
224224
fn test_condition() {
225-
let local_registry = fail::new_fail_group();
226-
local_registry.register_current();
225+
let local_registry = fail::create_registry();
226+
local_registry.register_current().unwrap();
227227

228228
let f = |_enabled| {
229229
fail_point!("condition", _enabled, |_| 2);
@@ -239,8 +239,8 @@ fn test_condition() {
239239

240240
#[test]
241241
fn test_list() {
242-
let local_registry = fail::new_fail_group();
243-
local_registry.register_current();
242+
let local_registry = fail::create_registry();
243+
local_registry.register_current().unwrap();
244244

245245
assert!(!fail::list().contains(&("list".to_string(), "off".to_string())));
246246
fail::cfg("list", "off").unwrap();
@@ -251,12 +251,12 @@ fn test_list() {
251251

252252
#[test]
253253
fn test_multiple_threads_cleanup() {
254-
let local_registry = fail::new_fail_group();
255-
local_registry.register_current();
254+
let local_registry = fail::create_registry();
255+
local_registry.register_current().unwrap();
256256

257257
let (tx, rx) = mpsc::channel();
258258
thread::spawn(move || {
259-
local_registry.register_current();
259+
local_registry.register_current().unwrap();
260260
fail::cfg("thread_point", "sleep(10)").unwrap();
261261
tx.send(()).unwrap();
262262
});
@@ -271,8 +271,8 @@ fn test_multiple_threads_cleanup() {
271271

272272
let (tx, rx) = mpsc::channel();
273273
let t = thread::spawn(move || {
274-
let local_registry = fail::new_fail_group();
275-
local_registry.register_current();
274+
let local_registry = fail::create_registry();
275+
local_registry.register_current().unwrap();
276276
fail::cfg("thread_point", "panic").unwrap();
277277
let l = fail::list();
278278
assert!(

0 commit comments

Comments
 (0)