From 57c72ab8469883fd761a9bba9c4f49771e48256f Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Sun, 24 Jan 2021 18:16:31 -0800 Subject: [PATCH 1/2] libtest: Wait for test threads to exit after they report completion Otherwise we can miss bugs where a test reports that it succeeded but then panics within a TLS destructor. Signed-off-by: Anders Kaseorg --- library/test/src/lib.rs | 59 +++++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs index 656d9669e81d2..f6b692404606d 100644 --- a/library/test/src/lib.rs +++ b/library/test/src/lib.rs @@ -25,6 +25,7 @@ #![feature(nll)] #![feature(available_concurrency)] #![feature(internal_output_capture)] +#![feature(option_unwrap_none)] #![feature(panic_unwind)] #![feature(staged_api)] #![feature(termination_trait_lib)] @@ -208,9 +209,15 @@ where use std::collections::{self, HashMap}; use std::hash::BuildHasherDefault; use std::sync::mpsc::RecvTimeoutError; + + struct RunningTest { + timeout: Instant, + join_handle: Option>, + } + // Use a deterministic hasher type TestMap = - HashMap>; + HashMap>; let tests_len = tests.len(); @@ -260,7 +267,11 @@ where let now = Instant::now(); let timed_out = running_tests .iter() - .filter_map(|(desc, timeout)| if &now >= timeout { Some(desc.clone()) } else { None }) + .filter_map( + |(desc, running_test)| { + if now >= running_test.timeout { Some(desc.clone()) } else { None } + }, + ) .collect(); for test in &timed_out { running_tests.remove(test); @@ -269,9 +280,9 @@ where } fn calc_timeout(running_tests: &TestMap) -> Option { - running_tests.values().min().map(|next_timeout| { + running_tests.values().map(|running_test| running_test.timeout).min().map(|next_timeout| { let now = Instant::now(); - if *next_timeout >= now { *next_timeout - now } else { Duration::new(0, 0) } + if next_timeout >= now { next_timeout - now } else { Duration::new(0, 0) } }) } @@ -280,7 +291,8 @@ where let test = remaining.pop().unwrap(); let event = TestEvent::TeWait(test.desc.clone()); notify_about_test_event(event)?; - run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No); + run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No) + .unwrap_none(); let completed_test = rx.recv().unwrap(); let event = TestEvent::TeResult(completed_test); @@ -291,11 +303,19 @@ where while pending < concurrency && !remaining.is_empty() { let test = remaining.pop().unwrap(); let timeout = time::get_default_test_timeout(); - running_tests.insert(test.desc.clone(), timeout); + let desc = test.desc.clone(); let event = TestEvent::TeWait(test.desc.clone()); notify_about_test_event(event)?; //here no pad - run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::Yes); + let join_handle = run_test( + opts, + !opts.run_tests, + test, + run_strategy, + tx.clone(), + Concurrent::Yes, + ); + running_tests.insert(desc, RunningTest { timeout, join_handle }); pending += 1; } @@ -323,8 +343,16 @@ where } } - let completed_test = res.unwrap(); - running_tests.remove(&completed_test.desc); + let mut completed_test = res.unwrap(); + let running_test = running_tests.remove(&completed_test.desc).unwrap(); + if let Some(join_handle) = running_test.join_handle { + if let Err(_) = join_handle.join() { + if let TrOk = completed_test.result { + completed_test.result = + TrFailedMsg("panicked after reporting success".to_string()); + } + } + } let event = TestEvent::TeResult(completed_test); notify_about_test_event(event)?; @@ -415,7 +443,7 @@ pub fn run_test( strategy: RunStrategy, monitor_ch: Sender, concurrency: Concurrent, -) { +) -> Option> { let TestDescAndFn { desc, testfn } = test; // Emscripten can catch panics but other wasm targets cannot @@ -426,7 +454,7 @@ pub fn run_test( if force_ignore || desc.ignore || ignore_because_no_process_support { let message = CompletedTest::new(desc, TrIgnored, None, Vec::new()); monitor_ch.send(message).unwrap(); - return; + return None; } struct TestRunOpts { @@ -441,7 +469,7 @@ pub fn run_test( monitor_ch: Sender, testfn: Box, opts: TestRunOpts, - ) { + ) -> Option> { let concurrency = opts.concurrency; let name = desc.name.clone(); @@ -469,9 +497,10 @@ pub fn run_test( let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32"); if concurrency == Concurrent::Yes && supports_threads { let cfg = thread::Builder::new().name(name.as_slice().to_owned()); - cfg.spawn(runtest).unwrap(); + Some(cfg.spawn(runtest).unwrap()) } else { runtest(); + None } } @@ -484,10 +513,12 @@ pub fn run_test( crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| { bencher.run(harness) }); + None } StaticBenchFn(benchfn) => { // Benchmarks aren't expected to panic, so we run them all in-process. crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn); + None } DynTestFn(f) => { match strategy { @@ -499,7 +530,7 @@ pub fn run_test( monitor_ch, Box::new(move || __rust_begin_short_backtrace(f)), test_run_opts, - ); + ) } StaticTestFn(f) => run_test_inner( desc, From b05788e859d3d553825eb114f07573f4839286b1 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Mon, 25 Jan 2021 12:19:25 -0800 Subject: [PATCH 2/2] libtest: Store pending timeouts in a deque This reduces the total complexity of checking timeouts from quadratic to linear, and should also fix an unwrap of None on completion of an already timed-out test. Signed-off-by: Anders Kaseorg --- library/test/src/lib.rs | 45 ++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs index f6b692404606d..3ff79eaea49ab 100644 --- a/library/test/src/lib.rs +++ b/library/test/src/lib.rs @@ -62,6 +62,7 @@ pub mod test { } use std::{ + collections::VecDeque, env, io, io::prelude::Write, panic::{self, catch_unwind, AssertUnwindSafe, PanicInfo}, @@ -211,7 +212,6 @@ where use std::sync::mpsc::RecvTimeoutError; struct RunningTest { - timeout: Instant, join_handle: Option>, } @@ -219,6 +219,11 @@ where type TestMap = HashMap>; + struct TimeoutEntry { + desc: TestDesc, + timeout: Instant, + } + let tests_len = tests.len(); let mut filtered_tests = filter_tests(opts, tests); @@ -262,25 +267,28 @@ where }; let mut running_tests: TestMap = HashMap::default(); + let mut timeout_queue: VecDeque = VecDeque::new(); - fn get_timed_out_tests(running_tests: &mut TestMap) -> Vec { + fn get_timed_out_tests( + running_tests: &TestMap, + timeout_queue: &mut VecDeque, + ) -> Vec { let now = Instant::now(); - let timed_out = running_tests - .iter() - .filter_map( - |(desc, running_test)| { - if now >= running_test.timeout { Some(desc.clone()) } else { None } - }, - ) - .collect(); - for test in &timed_out { - running_tests.remove(test); + let mut timed_out = Vec::new(); + while let Some(timeout_entry) = timeout_queue.front() { + if now < timeout_entry.timeout { + break; + } + let timeout_entry = timeout_queue.pop_front().unwrap(); + if running_tests.contains_key(&timeout_entry.desc) { + timed_out.push(timeout_entry.desc); + } } timed_out } - fn calc_timeout(running_tests: &TestMap) -> Option { - running_tests.values().map(|running_test| running_test.timeout).min().map(|next_timeout| { + fn calc_timeout(timeout_queue: &VecDeque) -> Option { + timeout_queue.front().map(|&TimeoutEntry { timeout: next_timeout, .. }| { let now = Instant::now(); if next_timeout >= now { next_timeout - now } else { Duration::new(0, 0) } }) @@ -305,7 +313,7 @@ where let timeout = time::get_default_test_timeout(); let desc = test.desc.clone(); - let event = TestEvent::TeWait(test.desc.clone()); + let event = TestEvent::TeWait(desc.clone()); notify_about_test_event(event)?; //here no pad let join_handle = run_test( opts, @@ -315,15 +323,16 @@ where tx.clone(), Concurrent::Yes, ); - running_tests.insert(desc, RunningTest { timeout, join_handle }); + running_tests.insert(desc.clone(), RunningTest { join_handle }); + timeout_queue.push_back(TimeoutEntry { desc, timeout }); pending += 1; } let mut res; loop { - if let Some(timeout) = calc_timeout(&running_tests) { + if let Some(timeout) = calc_timeout(&timeout_queue) { res = rx.recv_timeout(timeout); - for test in get_timed_out_tests(&mut running_tests) { + for test in get_timed_out_tests(&running_tests, &mut timeout_queue) { let event = TestEvent::TeTimeout(test); notify_about_test_event(event)?; }