-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Clarify the guarantees that ThreadId does and doesn't make. #84083
Conversation
The existing documentation does not spell out whether `ThreadId`s are unique during the lifetime of a thread or of a process. I had to examine the source code to realise (pleasingly!) that they're unique for the lifetime of a process. That seems worth documenting clearly, as it's a strong guarantee. Examining the way `ThreadId`s are created also made me realise that the `as_u64` method on `ThreadId` could be a trap for the unwary on those platforms where the platform's notion of a thread identifier is also a 64 bit integer (particularly if they happen to use a similar identifier scheme to `ThreadId`). I therefore think it's worth being even clearer that there's no relationship between the two.
(rust-highfive has picked a reviewer for you, use r? to override) |
r? @joshtriplett as we'll want a T-libs FCP on the new guarantees, but there's also relevant discussion in #67939 which may make us want to avoid making additional guarantees. |
I thought, based on the previous discussion, that we'd landed on not making these guarantees, so that we could just use the OS thread ID? |
I think I'm still of the same mind on this issue as when I wrote #67939 (comment): it doesn't really matter what we choose to do, IMHO, so long as we change (at least) one of the code and documentation (on the basis that the status quo is going to lead people astray). |
I've found myself increasingly enamored of more robust techniques to make Rust's standard library not care what environment it's called from. As a result, I like the idea of using the OS thread ID where possible, rather than inventing our own. |
I see that not necessarily for users' sake, but for implementations to avoid their own atomic thread counter. |
Worthy of consideration: are two crates which rely on All this to say that:
This means that if the std library does not guarantee this, the ecosystem will be split between people unaware of the issue doing The Wrong Thing™, and people reimplementing it themselves, inefficiently for OSes that actually gave stronger guarantees, and redundantly in between implementations, and even potentially incorrectly w.r.t. overflows. The standard library, on the other hand, to guarantee this, would just need a fallback module for this implementation, written once and correctly, that would only be pulled for the platforms unable to feature something better. So, while I respect the idea of reducing the maintenance burden for the standard library by not requiring
|
We discussed this again in today's @rust-lang/libs-api meeting. I decided to withdraw my objection, since you can only get the ID for a random non-Rust thread via @rfcbot merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
library/std/src/thread/mod.rs
Outdated
/// method on a [`Thread`]. | ||
/// A `ThreadId` is an opaque object that uniquely identifies each thread | ||
/// created during the lifetime of a process. `ThreadId`s are guaranteed not to | ||
/// be reused, even if a thread dies. `ThreadId`s are under the control of |
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.
/// be reused, even if a thread dies. `ThreadId`s are under the control of | |
/// be reused, even after thread terminates. `ThreadId`s are under the control of |
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.
I've used a minor variant of this in d66a9e1.
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r+ rollup |
📌 Commit d66a9e1 has been approved by |
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#84083 (Clarify the guarantees that ThreadId does and doesn't make.) - rust-lang#91593 (Remove unnecessary bounds for some Hash{Map,Set} methods) - rust-lang#92297 (Reduce compile time of rustbuild) - rust-lang#92332 (Add test for where clause order) - rust-lang#92438 (Enforce formatting for rustc_codegen_cranelift) - rust-lang#92463 (Remove pronunciation guide from Vec<T>) - rust-lang#92468 (Emit an error for `--cfg=)`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The existing documentation does not spell out whether
ThreadId
s are unique during the lifetime of a thread or of a process. I had to examine the source code to realise (pleasingly!) that they're unique for the lifetime of a process. That seems worth documenting clearly, as it's a strong guarantee.Examining the way
ThreadId
s are created also made me realise that theas_u64
method onThreadId
could be a trap for the unwary on those platforms where the platform's notion of a thread identifier is also a 64 bit integer (particularly if they happen to use a similar identifier scheme toThreadId
). I therefore think it's worth being even clearer that there's no relationship between the two.