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 <andersk@mit.edu>
This commit is contained in:
Anders Kaseorg 2021-01-24 18:16:31 -08:00
parent d3163e9655
commit 57c72ab846

View file

@ -25,6 +25,7 @@
#![feature(nll)] #![feature(nll)]
#![feature(available_concurrency)] #![feature(available_concurrency)]
#![feature(internal_output_capture)] #![feature(internal_output_capture)]
#![feature(option_unwrap_none)]
#![feature(panic_unwind)] #![feature(panic_unwind)]
#![feature(staged_api)] #![feature(staged_api)]
#![feature(termination_trait_lib)] #![feature(termination_trait_lib)]
@ -208,9 +209,15 @@ where
use std::collections::{self, HashMap}; use std::collections::{self, HashMap};
use std::hash::BuildHasherDefault; use std::hash::BuildHasherDefault;
use std::sync::mpsc::RecvTimeoutError; use std::sync::mpsc::RecvTimeoutError;
struct RunningTest {
timeout: Instant,
join_handle: Option<thread::JoinHandle<()>>,
}
// Use a deterministic hasher // Use a deterministic hasher
type TestMap = type TestMap =
HashMap<TestDesc, Instant, BuildHasherDefault<collections::hash_map::DefaultHasher>>; HashMap<TestDesc, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
let tests_len = tests.len(); let tests_len = tests.len();
@ -260,7 +267,11 @@ where
let now = Instant::now(); let now = Instant::now();
let timed_out = running_tests let timed_out = running_tests
.iter() .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(); .collect();
for test in &timed_out { for test in &timed_out {
running_tests.remove(test); running_tests.remove(test);
@ -269,9 +280,9 @@ where
} }
fn calc_timeout(running_tests: &TestMap) -> Option<Duration> { fn calc_timeout(running_tests: &TestMap) -> Option<Duration> {
running_tests.values().min().map(|next_timeout| { running_tests.values().map(|running_test| running_test.timeout).min().map(|next_timeout| {
let now = Instant::now(); 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 test = remaining.pop().unwrap();
let event = TestEvent::TeWait(test.desc.clone()); let event = TestEvent::TeWait(test.desc.clone());
notify_about_test_event(event)?; 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 completed_test = rx.recv().unwrap();
let event = TestEvent::TeResult(completed_test); let event = TestEvent::TeResult(completed_test);
@ -291,11 +303,19 @@ where
while pending < concurrency && !remaining.is_empty() { while pending < concurrency && !remaining.is_empty() {
let test = remaining.pop().unwrap(); let test = remaining.pop().unwrap();
let timeout = time::get_default_test_timeout(); 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()); let event = TestEvent::TeWait(test.desc.clone());
notify_about_test_event(event)?; //here no pad 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; pending += 1;
} }
@ -323,8 +343,16 @@ where
} }
} }
let completed_test = res.unwrap(); let mut completed_test = res.unwrap();
running_tests.remove(&completed_test.desc); 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); let event = TestEvent::TeResult(completed_test);
notify_about_test_event(event)?; notify_about_test_event(event)?;
@ -415,7 +443,7 @@ pub fn run_test(
strategy: RunStrategy, strategy: RunStrategy,
monitor_ch: Sender<CompletedTest>, monitor_ch: Sender<CompletedTest>,
concurrency: Concurrent, concurrency: Concurrent,
) { ) -> Option<thread::JoinHandle<()>> {
let TestDescAndFn { desc, testfn } = test; let TestDescAndFn { desc, testfn } = test;
// Emscripten can catch panics but other wasm targets cannot // 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 { if force_ignore || desc.ignore || ignore_because_no_process_support {
let message = CompletedTest::new(desc, TrIgnored, None, Vec::new()); let message = CompletedTest::new(desc, TrIgnored, None, Vec::new());
monitor_ch.send(message).unwrap(); monitor_ch.send(message).unwrap();
return; return None;
} }
struct TestRunOpts { struct TestRunOpts {
@ -441,7 +469,7 @@ pub fn run_test(
monitor_ch: Sender<CompletedTest>, monitor_ch: Sender<CompletedTest>,
testfn: Box<dyn FnOnce() + Send>, testfn: Box<dyn FnOnce() + Send>,
opts: TestRunOpts, opts: TestRunOpts,
) { ) -> Option<thread::JoinHandle<()>> {
let concurrency = opts.concurrency; let concurrency = opts.concurrency;
let name = desc.name.clone(); let name = desc.name.clone();
@ -469,9 +497,10 @@ pub fn run_test(
let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32"); let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32");
if concurrency == Concurrent::Yes && supports_threads { if concurrency == Concurrent::Yes && supports_threads {
let cfg = thread::Builder::new().name(name.as_slice().to_owned()); let cfg = thread::Builder::new().name(name.as_slice().to_owned());
cfg.spawn(runtest).unwrap(); Some(cfg.spawn(runtest).unwrap())
} else { } else {
runtest(); runtest();
None
} }
} }
@ -484,10 +513,12 @@ pub fn run_test(
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| { crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| {
bencher.run(harness) bencher.run(harness)
}); });
None
} }
StaticBenchFn(benchfn) => { StaticBenchFn(benchfn) => {
// Benchmarks aren't expected to panic, so we run them all in-process. // Benchmarks aren't expected to panic, so we run them all in-process.
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn); crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn);
None
} }
DynTestFn(f) => { DynTestFn(f) => {
match strategy { match strategy {
@ -499,7 +530,7 @@ pub fn run_test(
monitor_ch, monitor_ch,
Box::new(move || __rust_begin_short_backtrace(f)), Box::new(move || __rust_begin_short_backtrace(f)),
test_run_opts, test_run_opts,
); )
} }
StaticTestFn(f) => run_test_inner( StaticTestFn(f) => run_test_inner(
desc, desc,