"Cargo test" uses a thread pool in 1.33.0

[asomers]

Previous versions of Cargo would create and destroy a new thread for each test. Cargo 1.33.0 instead creates a thread pool and reuses the same thread for multiple tests. This can cause test failures for crates that implicitly rely on Cargo's old behavior for isolation. I'm not saying that the new behavior is wrong, but it deserves a mention in the release notes.

nix-rust/nix#1033

[RalfJung]

There is indeed a change in behavior, but not the way you suggest: behavior only changed for the case of a machine with exactly 1 core. Previously, it would spawn a thread per test and then basically immediately wait on that thread to finish; now instead it runs the tests in the main thread directly. There is no thread pool.

[asomers]

Perhaps there are two relevant PRs, because I absolutely do see thread-pool-like behavior. Just run strace -e clone cargo test on an n-core system, on almost any crate, for n > 1. With Rust and Cargo 1.33.0 there will be n clones. With older Rust there will be one clone per test.

Actually I just tried again and I'm not seeing that behavior anymore. Perhaps I did something stupid the first time. It may be that the only behavior change is for --test-threads=1.

[RalfJung]

Ah yes, indeed it's not about the number of cores but the concurrency level -- which defaults to the number of cores.

[Mark-Simulacrum]

Nominating for libs team -- I expect we'll probably want to not change anything here since the new behavior seems strictly better. However, as a regression, we should discuss.

[asomers]

I agree that the new behavior is better; Nix shouldn't have been relying on the old behavior. I just think that the new behavior should've been mentioned in the release notes.

[Mark-Simulacrum]

Yep, we missed it when compiling the release notes. If you'd like to propose a PR to master with this added to 1.33.0's notes that'd be appreciated!

[alexcrichton]

This was discussed during libs triage the other day and the conclusion was that adding this to the release notes is fine, so denominating.

[ipetkov]

Worth noting that this change may have led to #59122 and rust-lang/cargo#6746 which will break anyone trying to run tests for rust-1.33.0 from the source tarball, while running on a single core.

(Is there a workaround to telling rustbuild to tell cargo to use multiple test threads? using ./x.py test -j1 src/tools/cargo -- --test-threads=2 fails during invocation)

[RalfJung]

Oh gosh, I had no idea.

Maybe it's better to revert that part of the PR and add a cfg(miri) for avoiding concurrency?

[ehuss]

@ipetkov You can run RUST_TEST_THREADS=2 ./x.py test -j1 to force tests to use 2 threads as a workaround.

I looked at Cargo's testsuite, and it looks like it will be a bit tricky to work around. Because libtest does not have setup/teardown support, cargo's tests use thread-local storage to implement that. Any test that uses thread-local storage, and assumes no other tests reuse the tls, will likely fail when 1 thread is used.

It might be worth having some way to force "use main thread only" for use cases like rust-lang/cargo#5438 where things like gtk must run on the main thread. Otherwise, the only workaround is to use a custom test harness from scratch, which is currently awkward.

[ipetkov]

@ehuss thanks, that appears to unblock me for the moment!

@RalfJung as someone who maintains building rust from a source tarball, it would be best to not have silent, unexpected behavior if possible (e.g. like switching cores). Ideally whether or not cargo uses a threadpool shouldn't have any observable impacts (like declaring the wrong thread name when a panic happens), but I don't know enough about the internals to know if that's feasible

[ipetkov]

Original issue was for updating release notes (whose PR has been merged), but we should reopen since there are still some unanswered questions if the behavior is correct

[Mark-Simulacrum]

I'm going to re-nominate for the libs team since discussion has noted that in practice this change may have produced some rather painful breakage (i.e., no good workarounds). On the other hand, they seem similar to the problems we've long known about with per-test state (e.g. env_logger, etc.).

[alexcrichton]

The libs team discussed this and decided that we're still not going to rever this