libtest: Don't panic if unable to spawn thread

[camelid]

Instead just run the test synchronously.

[rust-highfive]

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

[camelid]

Should we print a message letting the user know?

[camelid]

I don't have a ton of experience with closures: what's the best way to fix this?

error[E0382]: use of moved value: `runtest`
   --> library/test/src/lib.rs:475:17
    |
473 |             if cfg.spawn(runtest).is_err() {
    |                          ------- value moved here
474 |                 // If we can't spawn a new thread, just run the test synchronously.
475 |                 runtest();
    |                 ^^^^^^^ value used here after move
    |
note: closure cannot be invoked more than once because it moves the variable `desc` out of its environment
   --> library/test/src/lib.rs:451:17
    |
451 |                 desc,
    |                 ^^^^

[jyn514]

@camelid there is not a simple way to fix it; run_test_in_process and spawn_test_subprocess both consume a TestDesc, but you don't know whether they will be called or not before spawn() fails. Actually that caught a different error: this should either assert the failure is WouldBlock or only call runtest() directly if the variant is WouldBlock.

The simplest way is probably to get run_test_in_process to take &mut TestDesc instead, but that might be tricky.

[camelid]

Actually that caught a different error: this should either assert the failure is WouldBlock or only call runtest() directly if the variant is WouldBlock.

Should be fixed now.

[jyn514]

@camelid can you remove 'may fix' from the commit message? This PR isn't actually fixing the same problem (although it might fix a related one).

[bors]

☔ The latest upstream changes (presumably #81367) made this pull request unmergeable. Please resolve the merge conflicts.

[andersk]

There’s probably a better way, but one brute-force strategy that works without much thinking would be to encase the closure in an Arc<Mutex<Option<…>>>:

let runtest = Arc::new(Mutex::new(Some(move || match opts.strategy {
    …
})));
…
cfg.spawn({
    let runtest = Arc::clone(&runtest);
    move || runtest.lock().unwrap().take().unwrap()()
})
…
runtest.lock().unwrap().take().unwrap()()

[ghost]

@camelid Any update? I would like to take over this if you run out of threads time.
EDIT: I think I have a working patch now: 97ca47f4708d85bf47b803377fc73ff39314c9f1.

There’s probably a better way, but one brute-force strategy that works without much thinking would be to encase the closure in an Arc<Mutex<Option<…>>>

I think Arc::get_mut and Mutex::get_mut could also be helpful.

[andersk]

Arc::get_mut and Mutex::get_mut require a unique reference and so can't be used from the thread.

[ghost]

Arc::get_mut and Mutex::get_mut require a unique reference and so can't be used from the thread.

I meant thay can be used after encountering ErrorKind::WouldBlock.

[jyn514]

EDIT: I think I have a working patch now: 97ca47f.

@hyd-dev I think it's fine to open a new PR and mention this one.

[jyn514]

err actually this is waiting-on-review, not waiting-on-author, so I don't know if there's any point to a new PR.

[camelid]

err actually this is waiting-on-review, not waiting-on-author, so I don't know if there's any point to a new PR.

This change doesn't work, so a new PR still makes sense 🙃

The reason it hasn't been reviewed is (1) it doesn't work and (2) withoutboats is taking a break.

[ghost]

New PR: #81546