Skip to content
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

Stabilize ThreadId::as_u64 #110738

Closed
wants to merge 2 commits into from
Closed

Conversation

ibraheemdev
Copy link
Member

Stabilization proposal

This PR proposes to stabilize the following API:

impl ThreadId {
    pub fn as_u64(&self) -> NonZeroU64;
}

Tracking issue: #67939

Implementation History

Design Considerations

The big question has been whether ThreadId should manage it's own internal counter or expose operating system IDs directly. #84083 added the guarantee that ThreadId is unique for the lifetime of the process, which makes managing IDs ourselves necessary. Note that this also makes returning a u64 necessary vs. usize.

The IDs being managed internally makes it trivial to guarantee that they are non-zero. This niche makes it possible to store an Option<ThreadId> as an AtomicU64.

Additionally, #110725 proposes applying a permutation sequence to avoid users relying on any internal order. The value is already documented as opaque, so it's not clear whether this is a stabilization concern.


Blocked on an FCP for stabilization.

@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@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 Apr 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2023

I think that we shouldn't apply a permutation to the output, and just return the sequential thread ID directly. This is the least surprising behavior.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 23, 2023

Team member @Amanieu 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2023
@the8472
Copy link
Member

the8472 commented Apr 23, 2023

I think the FCP is for the wrong team.

This is the least surprising behavior.

I'll argue that it's only surprising if you have an expectation that is inconsistent with what the API actually promises and that's all the more reason to change it. But we should discuss that on the other PR or the next libs meeting.

@Amanieu Amanieu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 23, 2023
@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2023

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Apr 23, 2023

@Amanieu proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2023
@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2023

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 23, 2023

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2023
@BurntSushi
Copy link
Member

The documentation for ThreadId::as_u64 does not look ideal to me. Here is the meat of it:

As noted in the documentation for the type itself, it is essentially an opaque ID, but is guaranteed to be unique for each thread. The returned value is entirely opaque – only equality testing is stable. Note that it is not guaranteed which values new threads will return, and this may change across Rust versions.

So the first sentence says that "it is essentially an opaque ID" and the second sentence says "the returned value is entirely opaque." This, to me, leaves me confused. To say something is "essentially an opaque ID" leaves a little room for ambiguity.

I think the docs should also call out why someone might want to use this routine. If one already has a ThreadId and that implements all of the usual traits, then why does one need a u64? (I suppose this is also me asking, "what is the motivation for this API in the first place." I'm somewhat assuming there is a good one, but it does appear to be unstated in this issue.)

@rfcbot concern docs

@BurntSushi
Copy link
Member

I suppose this is perhaps not a stabilization concern, but I think it would be good to at least have a discussion of permute versus non-permute before stabilizing. Just to make sure I am understanding things correctly, I think there are two choices on the table?

  • A new thread ID is computed by simply incrementing the previous thread ID by 1.
  • A new thread ID is computed by generating a pseudo random integer that is guaranteed to be unique from all other threads.

It seems to me like we should do the latter if we can. If we do the former, it seems inevitable that folks will rely on it implicitly. And then we'll be pragmatically locked into that choice. So to me, and the reason why it's important to discuss this during stabilization, is that if we're going to go with the former then it seems like we should just be realistic and add that as part of our API promise.

@rfcbot concern thread-id-generation

@m-ou-se m-ou-se added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 25, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Apr 25, 2023

I think it's fine to guarantee the IDs are generated in order. We already guarantee the IDs will never be recycled, so we need to generate our own IDs anyway.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 25, 2023

We briefly discussed this in today's libs-api meeting. We didn't reach a consensus on whether to permutate/randomize the IDs or to guarantee that they are sequentual, but we did think we should come to a conclusion on that before stabilizing this.

@m-ou-se m-ou-se added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023
@the8472
Copy link
Member

the8472 commented May 2, 2023

Concerns about guaranteeing ThreadID assignment by incrementing an atomic integer

  • we can't even guarantee that ID1 is the main thread
  • thread-creation is not well-ordered in a multi-threaded environment
  • may not scale to absurdly parallel systems due to cache traffic, such systems might want something similar to the snowflake algorithm)

Concerns about using an u64:

Assuming a hypothetical supercomputer with 131072 cores / compute units, 5GHz clock rate and a lightweight thread/task creation primitive that takes 100 instructions it's a ~6.5THz thread spawn rate -> 2^42 steps per second x 2^25 seconds per year -> more than 2^64 steps per year.
For now in practice no compute task runs that long or spawns tasks that frequently or would use the rust standard library for that, but that's cutting it awfully close considering we panic once the maximum is reached instead of doing a wrapping add.

@ChrisDenton
Copy link
Member

Ok so there are two issues being debated. Is u64 big enough, keeping in mind possible future developments? I dunno and I'll skip on that.

The other is if we should use randomness or permutations or something along those lines. I'm not sure I understand the threat model here. Concretely, what are the acual issues we're guarding against? We can't guarentee that the main thread is numbered 1 with non-rust entry points or pre-main shenanigans. I get that. We also can't guarentee that rust threads are spawned in any particular order or when/if non-rust spawned threads get a ThreadId. But we can state these caveats. I'm not clear what the actual risk is here that isn't already a risk? If someone is so minded, a simple transmute will get them a u64 today, on stable. And the current implentation is what it is.

@the8472
Copy link
Member

the8472 commented May 2, 2023

Concretely, what are the acual issues we're guarding against?

Users relying on implementation details instead of the contract. E.g. implementing "if main thread" as thread::current().id().as_u64() == 1 or hardcoding thread IDs in tests (based on observed values) or relying on iteration order of thread IDs put into a data structure.
General hyrum's law issues which are easily avoidable by adding a seeded permutation.

We also can't guarentee that rust threads are spawned in any particular order or when/if non-rust spawned threads get a ThreadId.

This isn't specific to non-rust threads. As soon as you have more than 1 thread and they're not synchronized you already have no global ordering anymore which means guaranteeing anything about ordering becomes nonsense.

If someone is so minded, a simple transmute will get them a u64 today, on stable.

Transmute is unsafe and has loud warnings on it though.

@the8472
Copy link
Member

the8472 commented May 3, 2023

From the zulip discussion: We need a finer breakdown of the use-cases. It seems possible that there are at least two non-overlapping ones that can be served by having separate APIs.

A) ensure that a value is only be used on the thread that created it (e.g. due to TLS access). If the value can live longer than the thread then preventing ID reuse is important. Checking value.thread_id == thread::current().id() may be sufficient for this and doesn't require as_u64()
B) stuffing the value into an atomic (usecase unclear, to identify waiting threads?). This might not require the value to be unique for the lifetime of a program.
C) fuzzy things such as tie-breakers, hashing etc

Neither the original PR (#67566, CC @Mark-Simulacrum ) nor the tracking issue are particularly illuminating.

Perhaps a Thread::os_id() or thread::current_os_thread_id() would already do the job for B and C.

@Andlon
Copy link

Andlon commented May 18, 2023

@the8472:

From the zulip discussion: We need a finer breakdown of the use-cases. It seems possible that there are at least two non-overlapping ones that can be served by having separate APIs.

A) ensure that a value is only be used on the thread that created it (e.g. due to TLS access). If the value can live longer than the thread then preventing ID reuse is important. Checking value.thread_id == thread::current().id() may be sufficient for this and doesn't require as_u64()
B) stuffing the value into an atomic (usecase unclear, to identify waiting threads?). This might not require the value to be unique for the lifetime of a program.
C) fuzzy things such as tie-breakers, hashing etc

Neither the original PR (#67566, CC @Mark-Simulacrum ) nor the tracking issue are particularly illuminating.

Perhaps a Thread::os_id() or thread::current_os_thread_id() would already do the job for B and C.

A use case that doesn't seem covered here is annotating log/tracing messages with a numeric ID that uniquely identifies the thread that the log/trace call was made on. An example where this is useful is constructing a per-thread hierarchical overview of tracing spans to show how much time was spent in different portions of a program.

Currently the only sound way to get some kind of unique ID for logging purposes is to use the Debug impl of ThreadId (which currently prints something like ThreadId(1), but the format cannot be relied on. Therefore, there is no stable format for unique thread identification. Currently the best we can do is to use string comparisons on the Debug output. as_u64 seems ideal for this purpose. See also this comment on the tracking issue for a related example.

@the8472
Copy link
Member

the8472 commented May 18, 2023

A use case that doesn't seem covered here is annotating log/tracing messages with a numeric ID that uniquely identifies the thread that the log/trace call was made on.

That doesn't answer the question of whether it has to be A) unique among the currently running threads or B) unique for the program lifetime. If a library is ok with printing the OS TID (as many libraries in other languages do) that would be A) while ThreadId implements B).

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 30, 2023
@m-ou-se m-ou-se 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 6, 2023
@ibraheemdev
Copy link
Member Author

For the use cases I was considering, the thread::current_os_thread_id() (using the address of a TLS variable) proposed by @the8472 makes more sense, leaving the door open to using u128 or other considerations for ThreadId, so I'll close this PR for now.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 21, 2023
@LunarLambda
Copy link

Are there any RFCs for exposing the native thread ID (oh god, as I was writing this I realised that "thread ID" is an incredibly dubious concept on Unix)? As mentioned in #115746 it would be useful to have some means of mapping between Rust ThreadIds and native thread IDs that will show up in debuggers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.