Skip to content

Documented thread hooks on panics and errors of thread spawning functions. #149927

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.

Sign up for GitHub

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

Jump to bottom
Open
AhoyISki wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Documented thread hooks on panics and errors of thread spawning functions. #149927

AhoyISki wants to merge 6 commits into from
+37 −5

Conversation

@AhoyISki
Copy link

@AhoyISki AhoyISki commented Dec 12, 2025 •
edited
Loading

This PR solves a concern regarding #132951

Considering that the feature has been on nightly for over a year, I would also like to see it stabilized, since no one has voiced any concerns about its current implementation.

@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 Dec 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2025

r? @joboet

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

joboet
joboet requested changes Dec 12, 2025
View reviewed changes
library/std/src/thread/builder.rs Outdated
///
/// Panics if a thread name was set and it contained null bytes.
///
/// Unlike the [`spawn`] free function, if it panics for this reason, the
Copy link
Member

@joboet joboet Dec 12, 2025

Choose a reason for hiding this comment

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

Unlike the spawn free function

thread::spawn calls Builder::spawn internally, so this is misleading – it's just that thread::spawn doesn't set a thread name and thus will never panic for this reason.

Copy link
Author

@AhoyISki AhoyISki Dec 12, 2025

Choose a reason for hiding this comment

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

Ok, I will rephrase.

library/std/src/thread/functions.rs Outdated
/// Panics if the OS fails to create a thread; use [`Builder::spawn`] to recover
/// from such errors.
///
/// Additionally, if hooks were added via [`add_spawn_hook`], they will still be
Copy link
Member

@joboet joboet Dec 12, 2025

Choose a reason for hiding this comment

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

"Additionally" isn't a good phrasing here, this isn't describing another panicking case, but rather adding more detail to the previous sentence. How about just combining the two paragraphs without any conjunction? Like

Panics if the OS fails to create a thread; use Builder::spawn to recover from such errors. If hooks...

library/std/src/thread/scoped.rs Outdated
/// Panics if the OS fails to create a thread; use [`Builder::spawn_scoped`]
/// to recover from such errors.
///
/// like the free [`spawn`] function, if hooks were added via [`add_spawn_hook`],
Copy link
Member

@joboet joboet Dec 12, 2025

Choose a reason for hiding this comment

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

This should then match the thread::spawn documentation exactly.

library/std/src/thread/scoped.rs Outdated
/// Panics if a thread name was set and it contained null bytes.
///
/// Unlike with [`Scope::spawn`], if it panics for this reason, the hooks
/// added by [`add_spawn_hook`] will _not_ be called.
Copy link
Member

@joboet joboet Dec 12, 2025

Choose a reason for hiding this comment

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

The same nit applies here.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@AhoyISki
Documentation rephrasing
a7185e9
@AhoyISki
Documentation rephrasing
8f0a1ce
@AhoyISki
Documentation rephrasing
d3a7384 
Reword documentation for clarity regarding spawn hooks.
@AhoyISki
Remove unused spawn reference in scoped.rs
9a7daa3 
Removed unused documentation reference to `spawn` in scoped thread.
@AhoyISki
Copy link
Author

AhoyISki commented Dec 13, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2025
@AhoyISki AhoyISki requested a review from joboet December 13, 2025 02:02
@joboet
Copy link
Member

joboet commented Dec 16, 2025

r? libs-api
This is just documenting the current behaviour, but I'd appreciate a signoff anyway.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 16, 2025
@rustbot rustbot assigned the8472 and unassigned joboet Dec 16, 2025
@bors
Copy link
Collaborator

bors commented Jan 1, 2026

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

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2026

⚠️ Warning ⚠️

  • The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

    You can start a rebase with the following commands:

    $ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git main
$ git push --force-with-lease

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2026
@rust-log-analyzer
Copy link
Collaborator

rust-log-analyzer commented Jan 11, 2026

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
............................................       (144/144)

======== tests/rustdoc-gui/notable-trait.goml ========

[ERROR] line 255: expected 0 elements, found 1: for command `assert-count: ("//*[@class='tooltip popover']", 0)`
    at <file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/struct.NotableStructWithLongName.html#method.create_an_iterator_from_read>

======== tests/rustdoc-gui/search-result-display.goml ========

[WARNING] line 39: Delta is 0 for "x", maybe try to use `compare-elements-position` instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joboet joboet joboet approved these changes

Assignees

@the8472 the8472

Labels

has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AhoyISki @rustbot @joboet @bors @rust-log-analyzer @the8472