Documented thread hooks on panics and errors of thread spawning functions. by AhoyISki · Pull Request #149927 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
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
| /// |
|---|
| /// Panics if a thread name was set and it contained null bytes. |
| /// |
| /// Unlike the [`spawn`] free function, if it panics for this reason, the |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the
spawnfree 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will rephrase.
| /// 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 |
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::spawnto recover from such errors. If hooks...
| /// 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`], |
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.
| /// 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. |
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.
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
Reminder, once the PR becomes ready for a review, use @rustbot ready.
Reword documentation for clarity regarding spawn hooks.
Removed unused documentation reference to spawn in scoped thread.
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
r? libs-api
This is just documenting the current behaviour, but I'd appreciate a signoff anyway.
Relevant to the library API team, which will review and decide on the PR/issue.
label
Labels
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
Relevant to the library API team, which will review and decide on the PR/issue.