Add a numeric ID to threads #29448

remram44 · Oct 29, 2015

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 · Oct 29, 2015

r? @brson

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

huonw · Oct 29, 2015

huonw reviewed Oct 29, 2015

View changes

huonw · Oct 29, 2015

src/libstd/thread/mod.rs

+    ///
+    /// This ID is unique across running threads, and a thread that has stopped
+    /// might see its ID reused.
+    pub fn id(&self) -> u32 {


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.)

retep998 · Oct 29, 2015

retep998 reviewed Oct 29, 2015

View changes

retep998 · Oct 29, 2015

src/libstd/sys/windows/thread.rs

@@ -78,6 +78,10 @@ impl Thread {
            c::Sleep(super::dur2timeout(dur))
        }
    }
+
+    pub fn id(&self) -> u32 {
+        42 // TODO


On Windows this is done either via GetCurrentThreadId or GetThreadId.

nagisa · Oct 29, 2015

nagisa reviewed Oct 29, 2015

View changes

nagisa · Oct 29, 2015

src/libstd/sys/unix/thread.rs

@@ -71,7 +71,7 @@ impl Thread {
        };

        extern fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
-            unsafe { start_thread(main); }
+            unsafe { start_thread(main, pthread_self() as u32); }


This is wrong.

POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted.

So should we use a typedef around what a thread ID is?

On Linux pthread_t is unsigned long, thus u64 on amd64. There is no portable way to get a numeric ID from pthreads (pthread_getthreadid_np is not portable).

I'm starting to think that using the address of the Inner struct is way simpler and would totally work (it's what I used for #29447) if we want "some kind of ID" and don't care for the OS-provided ID.

On Windows what I want is some sort of OS provided ID. Either the thread HANDLE or the thread ID which is a DWORD. Anything else would be useless to me as I wouldn't be able to use it in OS functions to manipulate that thread like queuing APCs to it.

Nevermind, I'm fine with this just providing some sort of unique numerical ID that has no meaning other than to distinguish threads. I'll work on a PR to add OS specific APIs for getting the HANDLE of a thread.

nagisa · Oct 29, 2015

nagisa reviewed Oct 29, 2015

View changes

nagisa · Oct 29, 2015

src/libstd/sys/common/thread.rs

    // Next, set up our stack overflow handler which may get triggered if we run
    // out of stack.
    let _handler = stack_overflow::Handler::new();

    // Finally, let's run some code.
-    Box::from_raw(main as *mut Box<FnBox()>)()
+    Box::from_raw(main as *mut Box<FnBox(u32)>)(id)


I do not understand why’d you want to pass the thread id as an argument into the thread instead of e.g. providing a static method on Thread which calls pthread_self.

alexcrichton · Oct 29, 2015

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 · Oct 29, 2015

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 · Nov 10, 2015

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

brson · 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 · Nov 12, 2015

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

If an id is exposed, it's basically guaranteed to be mistaken for the system id, so we probably want it to actually be.
We probably don't want to expose a literal integer because this isn't cross platform to all POSIX platforms.
This may want to expose an opaque ThreadId.
We also couldn't think of concrete use cases here.

alexcrichton · Dec 2, 2015

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

remram44 added some commits Oct 28, 2015

Add id() to sys Thread

3cc6d06

Add id to thread::Inner

95333c0

rust-highfive assigned brson Oct 29, 2015

remram44 added some commits Oct 28, 2015

Pass ids from sys-specific threads

6149a2a

Move id from Inner to Thread

Loading status checks…

ffd073d

huonw added I-nominated T-libs labels Oct 29, 2015

alexcrichton referenced this pull request Nov 12, 2015
Closed
Implement Eq for Thread #29447

alexcrichton removed the I-nominated label Nov 12, 2015

alexcrichton closed this Dec 2, 2015

nagisa referenced this pull request Dec 29, 2015
Closed
Make it possible to compare threads #1435

sagebind referenced this pull request Sep 8, 2016
Merged
Add ThreadId for comparing threads #36341

rust-lang/rust

Join GitHub today

Add a numeric ID to threads #29448

Conversation

remram44 commented Oct 29, 2015

remram44 added some commits Oct 28, 2015

rust-highfive assigned brson Oct 29, 2015

This comment has been minimized.

rust-highfive commented Oct 29, 2015

remram44 added some commits Oct 28, 2015

huonw added I-nominated T-libs labels Oct 29, 2015

huonw reviewed Oct 29, 2015 View changes

This comment has been minimized.

retep998 reviewed Oct 29, 2015 View changes

This comment has been minimized.

nagisa reviewed Oct 29, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa reviewed Oct 29, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Oct 29, 2015

This comment has been minimized.

retep998 commented Oct 29, 2015

This comment has been minimized.

bors commented Nov 10, 2015

This comment has been minimized.

brson commented Nov 11, 2015

alexcrichton referenced this pull request Nov 12, 2015

This comment has been minimized.

alexcrichton commented Nov 12, 2015

alexcrichton removed the I-nominated label Nov 12, 2015

This comment has been minimized.

alexcrichton commented Dec 2, 2015

alexcrichton closed this Dec 2, 2015

nagisa referenced this pull request Dec 29, 2015

sagebind referenced this pull request Sep 8, 2016

bors added a commit that referenced this pull request Oct 10, 2016

huonw reviewed Oct 29, 2015

View changes

retep998 reviewed Oct 29, 2015

View changes

nagisa reviewed Oct 29, 2015

View changes

nagisa reviewed Oct 29, 2015

View changes