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

Add a numeric ID to threads #29448

Closed
wants to merge 4 commits into from
Closed

Add a numeric ID to threads #29448

wants to merge 4 commits into from

Conversation

remram44
Copy link
Contributor

See #21507

Because we only get an ID once the thread has been spawned, and at that point the Inner struct is referenced from both the thread (which is gonna store it in the threadlocal thread_info) and the parent (which returns it), it was much easier to add it to Thread than to Inner. It is a single u32 so I'm not sure the memory usage is an issue, but it is a little awkward to have id in Thread and name in Inner?

Let me know.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@huonw huonw added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 29, 2015
///
/// This ID is unique across running threads, and a thread that has stopped
/// might see its ID reused.
pub fn id(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

If this PR is accepted, this will need to be marked #[unstable] and have a tracking issue opened. (See catch_panic in this module for an example.)

@alexcrichton
Copy link
Member

I think that there's a few pieces that may want to be tweaked here:

  • The id likely wants to be inside the Arc of the thread and just stored as an AtomicUsize. Both the parent and the child would initialize it.
  • The constructor Thread::new should perhaps not use 0 as the default as this means thread::current on every non-rust-spawned thread will have an id of 0 I believe.

I'm also not sure that we want to use the exact same ID as the underlying OS. As @nagisa points out we can't rely on pthread_t being an integer, so there may not always be an ID to assign.

@retep998
Copy link
Member

The constructor Thread::new should perhaps not use 0 as the default as this means thread::current on every non-rust-spawned thread will have an id of 0 I believe.

Getting the ID of the current thread is trivial, especially on Windows, so there shouldn't be any reason for thread::current to not return the correct ID.

@bors
Copy link
Contributor

bors commented Nov 10, 2015

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

@brson
Copy link
Contributor

brson commented Nov 11, 2015

Haven't looked at this in detail, but because the Rust thread type outlives the system thread, it's possible for comparison operations to produce incorrect results if the system reuses the thread id.

@alexcrichton
Copy link
Member

The libs team discussed this in triage today, and in addition to @brson's comment we also had thinking along the lines of:

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with the comments on this and #29447 addressed!

bors added a commit that referenced this pull request Oct 10, 2016
Add ThreadId for comparing threads

This adds the capability to store and compare threads with the current calling thread via a new struct, `std::thread::ThreadId`. Addresses the need outlined in issue #21507.

This avoids the need to add any special checks to the existing thread structs and does not rely on the system to provide an identifier for a thread, since it seems that this approach is unreliable and undesirable. Instead, this simply uses a lazily-created, thread-local `usize` whose value is copied from a global atomic counter. The code should be simple enough that it should be as much reliable as the `#[thread_local]` attribute it uses (however much that is).

`ThreadId`s can be compared directly for equality and have copy semantics.

Also see these other attempts:
- #29457
- #29448
- #29447

And this in the RFC repo: rust-lang/rfcs#1435
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

8 participants