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

Allow spawning threads after TLS destruction #138702

Merged
merged 4 commits into from
Mar 28, 2025

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Mar 19, 2025

Fixes #138696

@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2025
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with docs added (or maybe comment on the unstable tracking issue)

ChildSpawnHooks { hooks, to_run }
} else {
// TLS has been destroyed. Skip running the hooks.
// See https://github.com/rust-lang/rust/issues/138696
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document this in add_spawn_hook ("for every newly thread spawned"), maybe as a footnote or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems unresolved? I agree that it'd be good to document this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I responded to it below:

Added it to the tracking issue for now.

We might want to investigate exactly when this can and cannot happen before stabilizing (and documenting).

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 26, 2025

Added it to the tracking issue for now.

We might want to investigate exactly when this can and cannot happen before stabilizing (and documenting).

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Mar 26, 2025

📌 Commit 5912dad has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2025
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Mar 26, 2025
…mulacrum

Allow spawning threads after TLS destruction

Fixes rust-lang#138696
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang#130883 (Add environment variable query)
 - rust-lang#138672 (Avoiding calling queries when collecting active queries)
 - rust-lang#138702 (Allow spawning threads after TLS destruction)
 - rust-lang#138935 (Update wg-prio triagebot config)
 - rust-lang#138946 (Un-bury chapters from the chapter list in rustc book)
 - rust-lang#138964 (Implement lint against using Interner and InferCtxtLike in random compiler crates)
 - rust-lang#138977 (Don't deaggregate InvocationParent just to reaggregate it again)
 - rust-lang#138980 (Collect items referenced from var_debug_info)
 - rust-lang#138985 (Use the correct binder scope for elided lifetimes in assoc consts)

r? `@ghost`
`@rustbot` modify labels: rollup
@Zalathar
Copy link
Contributor

Looks like this failed in rollup: #138994 (comment)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 27, 2025
@Zalathar
Copy link
Contributor

This probably just needs a //@ needs-threads in the test to skip inappropriate targets.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Mar 27, 2025

Thanks. It failed with the following error message. #138994

Error

``` 2025-03-26T21:40:40.3315646Z failures: 2025-03-26T21:40:40.3315853Z 2025-03-26T21:40:40.3316147Z ---- [ui] tests/ui/thread-local/spawn-hook-atexit.rs stdout ---- 2025-03-26T21:40:40.3316528Z 2025-03-26T21:40:40.3325429Z error: test run failed! 2025-03-26T21:40:40.3325770Z status: exit status: 134 2025-03-26T21:40:40.3327164Z command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/thread-local/spawn-hook-atexit" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_TEST_THREADS="4" "/wasmtime-v19.0.0-x86_64-linux/wasmtime" "run" "-C" "cache=n" "--dir" "." "--env" "RUSTC_BOOTSTRAP" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/thread-local/spawn-hook-atexit/a.wasm" 2025-03-26T21:40:40.3328475Z stdout: none 2025-03-26T21:40:40.3328724Z --- stderr ------------------------------- 2025-03-26T21:40:40.3329095Z 2025-03-26T21:40:40.3329330Z thread 'main' panicked at /rustc/FAKE_PREFIX/library/std/src/thread/mod.rs:729:29: 2025-03-26T21:40:40.3329941Z failed to spawn thread: Error { kind: Unsupported, message: "operation not supported on this platform" } 2025-03-26T21:40:40.3330518Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 2025-03-26T21:40:40.3331181Z Error: failed to run main module `/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/thread-local/spawn-hook-atexit/a.wasm` 2025-03-26T21:40:40.3331635Z 2025-03-26T21:40:40.3331726Z Caused by: 2025-03-26T21:40:40.3331945Z 0: failed to invoke command default 2025-03-26T21:40:40.3332513Z 1: error while executing at wasm backtrace: 2025-03-26T21:40:40.3333117Z 0: 0x731d - a.wasm!__rustc[e71bbdf1327cd442]::__rust_start_panic 2025-03-26T21:40:40.3333847Z 1: 0x711a - a.wasm!__rustc[e71bbdf1327cd442]::rust_panic 2025-03-26T21:40:40.3334874Z 2: 0x70ed - a.wasm!std::panicking::rust_panic_with_hook::hf7253cc1326481e4 2025-03-26T21:40:40.3335606Z 3: 0x618f - a.wasm!std::panicking::begin_panic_handler::{{closure}}::he12880ca3e35ef7a 2025-03-26T21:40:40.3336306Z 4: 0x60fb - a.wasm!std::sys::backtrace::__rust_end_short_backtrace::h5892030c5e958e1f 2025-03-26T21:40:40.3336770Z 5: 0x6a80 - a.wasm!__rustc[e71bbdf1327cd442]::rust_begin_unwind 2025-03-26T21:40:40.3337185Z 6: 0xb81b - a.wasm!core::panicking::panic_fmt::hfecdcbf9788721e5 2025-03-26T21:40:40.3337691Z 7: 0xd3a8 - a.wasm!core::result::unwrap_failed::h0cf8ff721cbe3fe3 2025-03-26T21:40:40.3338103Z 8: 0xa88 - a.wasm!std::thread::spawn::h6b69bdde073e2268 2025-03-26T21:40:40.3338488Z 9: 0x1802 - a.wasm!spawn_hook_atexit::main::h872f7eff79614d37 2025-03-26T21:40:40.3338951Z 10: 0x5c6 - a.wasm!std::sys::backtrace::__rust_begin_short_backtrace::h0654a70c6c569708 2025-03-26T21:40:40.3339424Z 11: 0x5b9 - a.wasm!std::rt::lang_start::{{closure}}::h53d8d7c9849aa50e 2025-03-26T21:40:40.3339848Z 12: 0x37bb - a.wasm!std::rt::lang_start_internal::h824b1b60e0c6fb40 2025-03-26T21:40:40.3340199Z 13: 0x183b - a.wasm!__main_void 2025-03-26T21:40:40.3340566Z 14: 0x325 - a.wasm!_start 2025-03-26T21:40:40.3341079Z 2: wasm trap: wasm `unreachable` instruction executed 2025-03-26T21:40:40.3341648Z ------------------------------------------ 2025-03-26T21:40:40.3341982Z ```

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 27, 2025

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Mar 27, 2025

📌 Commit 3237b50 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2025
…lacrum

Allow spawning threads after TLS destruction

Fixes rust-lang#138696
@bors
Copy link
Collaborator

bors commented Mar 27, 2025

⌛ Testing commit 3237b50 with merge 46fc17b...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Mar 27, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 27, 2025
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 27, 2025

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Mar 27, 2025

📌 Commit 6c2161a has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2025
@bors
Copy link
Collaborator

bors commented Mar 27, 2025

⌛ Testing commit 6c2161a with merge 7586a9f...

@bors
Copy link
Collaborator

bors commented Mar 28, 2025

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 7586a9f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2025
@bors bors merged commit 7586a9f into rust-lang:master Mar 28, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 28, 2025
Copy link

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 3f55023 (parent) -> 7586a9f (this PR)

Test differences

Show 3 test diffs

Stage 1

  • [ui] tests/ui/thread-local/spawn-hook-atexit.rs: [missing] -> pass (J1)

Stage 2

  • [ui] tests/ui/thread-local/spawn-hook-atexit.rs: [missing] -> ignore (only executed when the target family is unix) (J0)
  • [ui] tests/ui/thread-local/spawn-hook-atexit.rs: [missing] -> pass (J2)

Job group index

  • J0: i686-msvc-1, x86_64-mingw-1, x86_64-msvc-1
  • J1: x86_64-gnu-llvm-18-3, x86_64-gnu-llvm-19-3
  • J2: aarch64-apple, aarch64-gnu, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, test-various, x86_64-apple-2, x86_64-gnu, x86_64-gnu-llvm-18-1, x86_64-gnu-llvm-18-2, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-nopt, x86_64-gnu-stable

Job duration changes

  1. x86_64-apple-2: 5649.4s -> 6510.9s (15.2%)
  2. dist-x86_64-apple: 8300.6s -> 9342.2s (12.5%)
  3. dist-x86_64-linux-alt: 6792.2s -> 7332.8s (8.0%)
  4. x86_64-apple-1: 7175.5s -> 7733.7s (7.8%)
  5. i686-mingw-3: 7776.0s -> 8367.0s (7.6%)
  6. dist-various-2: 3362.6s -> 3586.2s (6.7%)
  7. dist-x86_64-musl: 7891.9s -> 8164.5s (3.5%)
  8. dist-armv7-linux: 5290.0s -> 5451.4s (3.1%)
  9. dist-x86_64-netbsd: 5200.3s -> 5355.1s (3.0%)
  10. dist-x86_64-linux: 5437.7s -> 5598.0s (2.9%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7586a9f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 777.846s -> 778.708s (0.11%)
Artifact size: 365.94 MiB -> 365.91 MiB (-0.01%)

@m-ou-se m-ou-se deleted the spawn-in-atexit branch March 28, 2025 07:13
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
…lacrum

Allow spawning threads after TLS destruction

Fixes rust-lang#138696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::thread::spawn: thread-local storage panics
9 participants