Skip to content

Conversation

@joboet
Copy link
Member

@joboet joboet commented Nov 9, 2025

Almost all functionality in std::thread is currently implemented in thread/mod.rs, resulting in a huge file with more than 2000 lines and multiple, interoperating unsafe sections. This PR splits the file up into multiple different private modules, each implementing mostly independent parts of the functionality. The only remaining unsafe interplay is that of the lifecycle and scope modules, the spawn_scoped implementation relies on the live thread count being updated correctly by the lifecycle module.

This PR contains no functional changes and only moves code around for the most part, with a few notable exceptions:

  • with_current_name is moved to the already existing current module and now uses the name method instead of calculating the name from private fields. The old code was just a reimplementation of that method anyway.
  • The private JoinInner type used to implement both join handles now has some more methods (is_finished, thread and the AsInner/IntoInner implementations) to avoid having to expose private fields and their invariants.
  • The private spawn_unchecked_ (note the underscore) method of Builder is now a freestanding function in lifecycle.

The rest of the changes are just visibility annotations.

I realise this PR ended up quite large – let me know if there is anyway I can aid the review process.

Edit: I've simplified the diff by adding an intermediate commit that creates all the new files by duplicating mod.rs. The actual changes in the second commit thus appear to delete the non-relevant parts from the respective file.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2025

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
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

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
 Documenting std v0.0.0 (/checkout/library/std)
error: unresolved link to `park`
   --> library/std/src/thread/thread.rs:109:27
    |
109 |     /// Like the public [`park`], but callable on any handle. This is used to
    |                           ^^^^ no item named `park` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
    = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(rustdoc::broken_intra_doc_links)]`

error: unresolved link to `park_timeout`
   --> library/std/src/thread/thread.rs:118:27
    |
118 |     /// Like the public [`park_timeout`], but callable on any handle. This is
    |                           ^^^^^^^^^^^^ no item named `park_timeout` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: could not document `std`
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] std test:false 8.635
Bootstrap failed while executing `doc library --stage 1`
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo doc --target x86_64-unknown-linux-gnu -Zbinary-dep-depinfo -j 4 -Zroot-dir=/checkout --locked --color always --release -p alloc -p compiler_builtins -p core -p panic_abort -p panic_unwind -p proc_macro -p rustc-std-workspace-core -p std -p std_detect -p sysroot -p test -p unwind --features 'backtrace panic-unwind compiler-builtins-c' --manifest-path /checkout/library/sysroot/Cargo.toml --no-deps --target-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/doc -Zskip-rustdoc-fingerprint -Zrustdoc-map [workdir=/checkout]` failed with exit code 101
Created at: src/bootstrap/src/core/build_steps/doc.rs:781:21
Executed at: src/bootstrap/src/core/build_steps/doc.rs:814:22

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:06
  local time: Mon Nov 10 08:58:21 UTC 2025
  network time: Mon, 10 Nov 2025 08:58:22 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have some module-level docs for this, the name isn't exactly self-explaining.

@ChrisDenton
Copy link
Member

ChrisDenton commented Nov 10, 2025

Hm, I'm going to nominate this for @rust-lang/libs. Not because it particularly needs discussion but just for awareness. If anyone has anything they'd like to change/add about this PR I'd prefer they speak up now then have more churn after this is merged.

I've not finished considering it yet but my initial thought is that it seems to have swung a bit too heavily from (almost) everything in one file to (almost) everything in separate files. For example, JoinHandle is essentially a small shim that simply forwards to the underlying implementation and the only unsafe is the Send and Sync impls. Builder is somewhat similarly trivial.

@ChrisDenton ChrisDenton added the I-libs-nominated Nominated for discussion during a libs team meeting. label Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I-libs-nominated Nominated for discussion during a libs team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

5 participants