Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Cargo test" uses a thread pool in 1.33.0 #58907

Closed
asomers opened this issue Mar 4, 2019 · 14 comments
Closed

"Cargo test" uses a thread pool in 1.33.0 #58907

asomers opened this issue Mar 4, 2019 · 14 comments
Labels
A-libtest Area: `#[test]` / the `test` library regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@asomers
Copy link
Contributor

asomers commented Mar 4, 2019

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

@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-libtest Area: `#[test]` / the `test` library labels Mar 4, 2019
@RalfJung
Copy link
Member

RalfJung commented Mar 5, 2019

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
Copy link
Contributor Author

asomers commented Mar 5, 2019

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
Copy link
Member

RalfJung commented Mar 5, 2019

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

@Mark-Simulacrum Mark-Simulacrum added I-nominated regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Mar 5, 2019
@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor Author

asomers commented Mar 6, 2019

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
Copy link
Member

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
Copy link
Member

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

asomers added a commit to asomers/rust that referenced this issue Mar 9, 2019
@ipetkov
Copy link
Contributor

ipetkov commented Mar 14, 2019

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
Copy link
Member

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
Copy link
Contributor

ehuss commented Mar 14, 2019

@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
Copy link
Contributor

ipetkov commented Mar 15, 2019

@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

kennytm added a commit to kennytm/rust that referenced this issue Mar 16, 2019
@ipetkov
Copy link
Contributor

ipetkov commented Mar 16, 2019

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
Copy link
Member

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.).

pietroalbini pushed a commit to pietroalbini/rust that referenced this issue Mar 19, 2019
@alexcrichton
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants