Disable thread support in libtest if RUST_TEST_THREADS=1

[ianks]

After #103681, each test now runs in it's own thread if the platform supports it. This was true even if RUST_TEST_THREADS=1. Unfortunately, this causes some subtle breakage for certain libraries that may rely on thread-local storage being shared across tests (as is the case for libruby and Julia, and probably more).

The feature never intended to cause any breakage, so restoring the previous behavior should be OK. I considered adding a new option, such as RUST_TEST_THREADS=0, which would allow both the new and previous behavior to exist side-by-side. But the idea of 0 threads felt... wrong. So I think this is the best way forward.

This is my first rust PR, so please let me know if I missed anything! ❤️

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[Mark-Simulacrum]

r? @thomcc

It feels like we might want this to be a separate knob, not related to threads=1...

[the8472]

It feels like we might want this to be a separate knob, not related to threads=1...

Agreed. Sometimes testing with N > 1 threads could also benefit from reusing threads instead of running a thread-per-test, e.g. when loads of thread-local caches or similar things need to be initialized.
I don't think we ever had thread pooling in libtest, but choosing a generic option name would allow it to cover both cases (all tests in 1 thread and all tests on pooled threads).

[thomcc]

This kinda feels like a libs-api decision, regarding how this "should" work.

[thomcc]

Actually, I've realized that promising this and doing it in this manner would lock us into never fixing some bugs in libtest (like having no slow test notification on machines with 1 core). Let me dig up some old code and make a counter-proposal.

[uazu]

As a vote in favour of the intent of this change (although I'm not following the details):

I have a case where I need to test code that effectively locks future invocations of that same code to the same thread. This is to make internal use of global variables by the code sound. This is for an optional feature-based optimisation in the crate, but the optimisation is worthwhile for the cases where it is used, saving both memory and CPU. It tested fine until 1.67, but since then I can't test it because it immediately panics when it notices that it's running on another thread.

Making RUST_TEST_THREADS=1 use just a single thread again would fix my problem. Right now I have to use a hacky workaround, using --list to get a list of tests and running them one by one.

[bors]

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

[knopp]

Rust default test harness is very much unusable for anything on macOS that requires execution on main thread. Unfortunately main thread is special (so that on some places there are actually checks for pthread_main_np, i.e. in libdispatch. It would be immensely helpful if there was way to run tests with the default harness directly on main thread.

[RalfJung]

The feature never intended to cause any breakage, so restoring the previous behavior should be OK.

FWIW running it all in one thread was also never intended to be a feature -- previous behavior (Rust 1.0 - ~1.30) was to always spawn threads; #56243 did the "run in a single thread" thing as a hack to make cargo miri test work.

So, neither #56243 nor #103681 was intended to be more than an internal refactor, and neither should be read as indicating intent. Forcing a single thread has disadvantages (e.g. #59122).

[tiagolobocastro]

Something like this would be useful for us, as we found out yesterday during rust update.
We have an extern dependency which allows 1-time init for the process lifetime and we have to use the same thread everytime we interact with it.. so not much we can do from our side.

However seems other things also get broken by the previous behaviour which is a no-win situation.
I would suggest reverting back to previous behaviour and try to come up with an approach that would work better overall, maybe exposing a separate env variable or perhaps a threads flavour to the test macro?

#[test(flavour = "static_thread")]
fn test_xyz() {}

[the8472]

If the library has a requirement to run on a specific thread then it should do what many UI libraries do and provide something like fn dispatch_to_ui_thread<R, F>(f: F) -> R where F: FnOnce() -> R + Send, R: Send that submits it via a channel it to the initialized thread and waits for the result to be returned.

Then tests that interact with that library can serialize themselves by dispatching to that thread. Other tests that don't touch that part can still run in parallel.

None of that requires any changes to libtest. This is a problem of libraries that have additional requirements but do not provide the necessary facilities to meet those requirements in their API.

[bjorn3]

None of that requires any changes to libtest.

It does require a change to free up the main thread for use by the ui framework. In many cases the ui thread can only be the main thread, but it is currently being used by libtest so nobody else can run anything on it.

[thomcc]

Yeah, apple provides those functions but a function you sent to that queue would never get run (and if you blocked on it, you'd deadlock), because libtest doesn't start the main loop.

Overall I think this is something that should be discussed by t-testing.

[knopp]

This is a problem of libraries that have additional requirements but do not provide the necessary facilities to meet those requirements in their API.

The API is all there - I can easily dispatch things to main thead on macOS (i.e. dispatch_async or CFRunLoopAddSource) but that will not help because the main thread is stuck in libtest instead of running the run loop.

[the8472]

It does require a change to free up the main thread for use by the ui framework.

Last time (it has been a few years) I had to deal with these kinds of issues the framework had a way to initialize the UI event loop on something that was not the program's first/main thread.

The API is all there - I can easily dispatch things to main thead on macOS (i.e. dispatch_async or CFRunLoopAddSource) but that will not help because the main thread is stuck in libtest instead of running the run loop.

Ok, I haven't dealt with macos. Does it not have that?

[knopp]

Ok, I haven't dealt with macos. Does it not have that?

Unfortunately no. There are hard-coded assumptions about main thread in various places.

https://github.com/opensource-apple/CF/blob/3cc41a76b1491f50813e28a4ec09954ffa359e6f/CFRunLoop.c#L1482-L1487

CFRunLoopRef CFRunLoopGetMain(void) {
    CHECK_FOR_FORK();
    static CFRunLoopRef __main = NULL; // no retain needed
    if (!__main) __main = _CFRunLoopGet0(pthread_main_thread_np()); // no CAS needed
    return __main;
}

There are similar checks all over AppKit, libdispatch, etc. too. The only way for things to work properly is to keep running the run-loop (CFRunLoopRun()) on main thread (first created thread - for which pthread_main_thread_np() returns 1).

[thomcc]

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

[m-ou-se]

I don't think we should make this behaviour implicit on RUST_TEST_THREADS=1. We might want another environment variable or a specical attribute to run a test on the main thread for example, but that's a design question separate from this PR, that will require an ACP and should probably be designed together with the testing team.

@rfcbot close

[rfcbot]

Team member @m-ou-se has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @cuviper
• @joshtriplett
• @m-ou-se
• @the8472
• @thomcc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[m-ou-se]

cc @rust-lang/testing-devex

[epage]

I agree with the disposition to close based on what I saw in the thread.

Another workaround for this is to use a custom test harness (libtest-mimic can help). Of course, I'd love to improve the experience of using a custom test harness. If someone would like to help out in that effort, come on over to https://github.com/rust-lang/libtest-next