Skip to content

Commit e0414d5

Browse files
Rollup merge of #81367 - andersk:join-test-threads, r=dtolnay
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. Example: ```rust use std::thread::sleep; use std::time::Duration; struct Foo; impl Drop for Foo { fn drop(&mut self) { sleep(Duration::from_secs(1)); panic!() } } thread_local!(static FOO: Foo = Foo); #[test] pub fn test() { FOO.with(|_| {}); } ``` Before this fix, `cargo test` incorrectly reports success. ```console $ cargo test Finished test [unoptimized + debuginfo] target(s) in 0.01s Running target/debug/deps/panicking_test-85130fa46b54f758 running 1 test test test ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s $ echo $? 0 ``` After this fix, the failure is visible. (The entire process is aborted due to #24479.) ```console $ cargo test Finished test [unoptimized + debuginfo] target(s) in 0.01s Running target/debug/deps/panicking_test-76180625bc2ee3c9 running 1 test thread 'test' panicked at 'explicit panic', src/main.rs:9:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace fatal runtime error: failed to initiate panic, error 5 error: test failed, to rerun pass '--bin panicking-test' Caused by: process didn't exit successfully: `/tmp/panicking-test/target/debug/deps/panicking_test-76180625bc2ee3c9 --nocapture` (signal: 6, SIGABRT: process abort signal) $ echo $? 101 ```
2 parents 75563f1 + 57c72ab commit e0414d5

File tree

1 file changed

+45
-14
lines changed

1 file changed

+45
-14
lines changed

library/test/src/lib.rs

+45-14
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#![feature(nll)]
2626
#![feature(available_concurrency)]
2727
#![feature(internal_output_capture)]
28+
#![feature(option_unwrap_none)]
2829
#![feature(panic_unwind)]
2930
#![feature(staged_api)]
3031
#![feature(termination_trait_lib)]
@@ -208,9 +209,15 @@ where
208209
use std::collections::{self, HashMap};
209210
use std::hash::BuildHasherDefault;
210211
use std::sync::mpsc::RecvTimeoutError;
212+
213+
struct RunningTest {
214+
timeout: Instant,
215+
join_handle: Option<thread::JoinHandle<()>>,
216+
}
217+
211218
// Use a deterministic hasher
212219
type TestMap =
213-
HashMap<TestDesc, Instant, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
220+
HashMap<TestDesc, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
214221

215222
let tests_len = tests.len();
216223

@@ -260,7 +267,11 @@ where
260267
let now = Instant::now();
261268
let timed_out = running_tests
262269
.iter()
263-
.filter_map(|(desc, timeout)| if &now >= timeout { Some(desc.clone()) } else { None })
270+
.filter_map(
271+
|(desc, running_test)| {
272+
if now >= running_test.timeout { Some(desc.clone()) } else { None }
273+
},
274+
)
264275
.collect();
265276
for test in &timed_out {
266277
running_tests.remove(test);
@@ -269,9 +280,9 @@ where
269280
}
270281

271282
fn calc_timeout(running_tests: &TestMap) -> Option<Duration> {
272-
running_tests.values().min().map(|next_timeout| {
283+
running_tests.values().map(|running_test| running_test.timeout).min().map(|next_timeout| {
273284
let now = Instant::now();
274-
if *next_timeout >= now { *next_timeout - now } else { Duration::new(0, 0) }
285+
if next_timeout >= now { next_timeout - now } else { Duration::new(0, 0) }
275286
})
276287
}
277288

@@ -280,7 +291,8 @@ where
280291
let test = remaining.pop().unwrap();
281292
let event = TestEvent::TeWait(test.desc.clone());
282293
notify_about_test_event(event)?;
283-
run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No);
294+
run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No)
295+
.unwrap_none();
284296
let completed_test = rx.recv().unwrap();
285297

286298
let event = TestEvent::TeResult(completed_test);
@@ -291,11 +303,19 @@ where
291303
while pending < concurrency && !remaining.is_empty() {
292304
let test = remaining.pop().unwrap();
293305
let timeout = time::get_default_test_timeout();
294-
running_tests.insert(test.desc.clone(), timeout);
306+
let desc = test.desc.clone();
295307

296308
let event = TestEvent::TeWait(test.desc.clone());
297309
notify_about_test_event(event)?; //here no pad
298-
run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::Yes);
310+
let join_handle = run_test(
311+
opts,
312+
!opts.run_tests,
313+
test,
314+
run_strategy,
315+
tx.clone(),
316+
Concurrent::Yes,
317+
);
318+
running_tests.insert(desc, RunningTest { timeout, join_handle });
299319
pending += 1;
300320
}
301321

@@ -323,8 +343,16 @@ where
323343
}
324344
}
325345

326-
let completed_test = res.unwrap();
327-
running_tests.remove(&completed_test.desc);
346+
let mut completed_test = res.unwrap();
347+
let running_test = running_tests.remove(&completed_test.desc).unwrap();
348+
if let Some(join_handle) = running_test.join_handle {
349+
if let Err(_) = join_handle.join() {
350+
if let TrOk = completed_test.result {
351+
completed_test.result =
352+
TrFailedMsg("panicked after reporting success".to_string());
353+
}
354+
}
355+
}
328356

329357
let event = TestEvent::TeResult(completed_test);
330358
notify_about_test_event(event)?;
@@ -415,7 +443,7 @@ pub fn run_test(
415443
strategy: RunStrategy,
416444
monitor_ch: Sender<CompletedTest>,
417445
concurrency: Concurrent,
418-
) {
446+
) -> Option<thread::JoinHandle<()>> {
419447
let TestDescAndFn { desc, testfn } = test;
420448

421449
// Emscripten can catch panics but other wasm targets cannot
@@ -426,7 +454,7 @@ pub fn run_test(
426454
if force_ignore || desc.ignore || ignore_because_no_process_support {
427455
let message = CompletedTest::new(desc, TrIgnored, None, Vec::new());
428456
monitor_ch.send(message).unwrap();
429-
return;
457+
return None;
430458
}
431459

432460
struct TestRunOpts {
@@ -441,7 +469,7 @@ pub fn run_test(
441469
monitor_ch: Sender<CompletedTest>,
442470
testfn: Box<dyn FnOnce() + Send>,
443471
opts: TestRunOpts,
444-
) {
472+
) -> Option<thread::JoinHandle<()>> {
445473
let concurrency = opts.concurrency;
446474
let name = desc.name.clone();
447475

@@ -469,9 +497,10 @@ pub fn run_test(
469497
let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32");
470498
if concurrency == Concurrent::Yes && supports_threads {
471499
let cfg = thread::Builder::new().name(name.as_slice().to_owned());
472-
cfg.spawn(runtest).unwrap();
500+
Some(cfg.spawn(runtest).unwrap())
473501
} else {
474502
runtest();
503+
None
475504
}
476505
}
477506

@@ -484,10 +513,12 @@ pub fn run_test(
484513
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| {
485514
bencher.run(harness)
486515
});
516+
None
487517
}
488518
StaticBenchFn(benchfn) => {
489519
// Benchmarks aren't expected to panic, so we run them all in-process.
490520
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn);
521+
None
491522
}
492523
DynTestFn(f) => {
493524
match strategy {
@@ -499,7 +530,7 @@ pub fn run_test(
499530
monitor_ch,
500531
Box::new(move || __rust_begin_short_backtrace(f)),
501532
test_run_opts,
502-
);
533+
)
503534
}
504535
StaticTestFn(f) => run_test_inner(
505536
desc,

0 commit comments

Comments
 (0)