-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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.
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
base: main
Are you sure you want to change the base?
Conversation
… spawning functions.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...
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`], |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
|
Reminder, once the PR becomes ready for a review, use |
Reword documentation for clarity regarding spawn hooks.
Removed unused documentation reference to `spawn` in scoped thread.
|
@rustbot ready |
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.