-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Implement Thread::os_id
#160219
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?
Implement Thread::os_id
#160219
Changes from 1 commit
c496571
84d2f10
3a3ff33
5c8cb4e
42cdf8c
71be0e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ use crate::fmt; | |
| use crate::pin::Pin; | ||
| use crate::sync::Arc; | ||
| use crate::sys::sync::Parker; | ||
| use crate::sys::thread as imp; | ||
| use crate::time::Duration; | ||
|
|
||
| // This module ensures private fields are kept private, which is necessary to enforce the safety requirements. | ||
|
|
@@ -40,6 +41,59 @@ mod thread_name_string { | |
|
|
||
| use thread_name_string::ThreadNameString; | ||
|
|
||
| // The handle of a spawned thread exists before the thread does, so the thread | ||
| // stores its own id once it starts running, hence the atomic. 0 means "not known". | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing guarantees that the OS's ID is non-zero, we really shouldn't use zero as a sentinel.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched to OnceLock: write-once without a sentinel, and it needs no 64-bit atomic so the cfg_select is gone too. Waiting would need the child to always store the value, so OnceLock<Option>. |
||
| // | ||
| // Of the platform calls behind `current_os_id`, only Apple's yields a `uint64_t`, | ||
| // and every Apple target has 64-bit atomics, so `usize` loses nothing on the | ||
| // second arm. | ||
| cfg_select! { | ||
| target_has_atomic = "64" => { | ||
| use crate::sync::atomic::{Atomic, AtomicU64, Ordering::Relaxed}; | ||
|
|
||
| struct OsId(Atomic<u64>); | ||
|
|
||
| impl OsId { | ||
| const fn unknown() -> Self { | ||
| Self(AtomicU64::new(0)) | ||
| } | ||
|
|
||
| fn get(&self) -> Option<u64> { | ||
| match self.0.load(Relaxed) { | ||
| 0 => None, | ||
| id => Some(id), | ||
| } | ||
| } | ||
|
|
||
| fn set(&self, id: u64) { | ||
| self.0.store(id, Relaxed); | ||
| } | ||
| } | ||
| } | ||
| _ => { | ||
| use crate::sync::atomic::{Atomic, AtomicUsize, Ordering::Relaxed}; | ||
|
|
||
| struct OsId(Atomic<usize>); | ||
|
|
||
| impl OsId { | ||
| const fn unknown() -> Self { | ||
| Self(AtomicUsize::new(0)) | ||
| } | ||
|
|
||
| fn get(&self) -> Option<u64> { | ||
| match self.0.load(Relaxed) { | ||
| 0 => None, | ||
| id => Some(id as u64), | ||
| } | ||
| } | ||
|
|
||
| fn set(&self, id: u64) { | ||
| self.0.store(id as usize, Relaxed); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// The internal representation of a `Thread` handle | ||
| /// | ||
| /// We explicitly set the alignment for our guarantee in Thread::into_raw. This | ||
|
|
@@ -49,6 +103,7 @@ use thread_name_string::ThreadNameString; | |
| struct Inner { | ||
| name: Option<ThreadNameString>, | ||
| id: ThreadId, | ||
| os_id: OsId, | ||
| parker: Parker, | ||
| } | ||
|
|
||
|
|
@@ -103,13 +158,25 @@ impl Thread { | |
| let ptr = Arc::get_mut_unchecked(&mut arc).as_mut_ptr(); | ||
| (&raw mut (*ptr).name).write(name); | ||
| (&raw mut (*ptr).id).write(id); | ||
| (&raw mut (*ptr).os_id).write(OsId::unknown()); | ||
| Parker::new_in_place(&raw mut (*ptr).parker); | ||
| Pin::new_unchecked(arc.assume_init()) | ||
| }; | ||
|
|
||
| Thread { inner } | ||
| } | ||
|
|
||
| /// Records the OS id of the calling thread in this handle. | ||
| /// | ||
| /// May only be called from the thread to which this handle belongs. A | ||
| /// spawned thread does this itself once it starts running, since its handle | ||
| /// already exists by then. | ||
| pub(crate) fn set_os_id_to_current(&self) { | ||
| if let Some(os_id) = imp::current_os_id() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The SGX impl of Specifically the sequence is:
(On the spawn_unchecked path we'd set_current before we hit this code). I think the two fixes are either (a) we modify cc @jethrogb @raoulstrackx @aditijannu (sgx target maintainers), in case you have an opinion on the "OS" IDs of threads for the target (https://doc.rust-lang.org/nightly/rustc/platform-support/x86_64-fortanix-unknown-sgx.html).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That I went through every
Not sure that's the right place to document this. On reordering: putting set_os_id after initializing the thread-local pointer to Arc, could help to drop these constraint, so some platforms may use It also wouldn't cover the |
||
| self.inner.os_id.set(os_id); | ||
| } | ||
| } | ||
|
|
||
| /// Like the public [`park`], but callable on any handle. This is used to | ||
| /// allow parking in TLS destructors. | ||
| /// | ||
|
|
@@ -204,6 +271,35 @@ impl Thread { | |
| self.inner.id | ||
| } | ||
|
|
||
| /// Gets the id the operating system gave this thread, if it has one that can | ||
| /// be read. | ||
| /// | ||
| /// This is the id `ps`, `top`, a debugger or a crash log shows, unlike | ||
| /// [`ThreadId`], which is internal to Rust and unrelated to it. `None` means | ||
| /// the platform has no such id or offers no way to read it, or that the | ||
| /// thread has not started running yet. | ||
| /// | ||
| /// The operating system may hand the same id to a later thread once this one | ||
| /// exits, so it does not name a thread uniquely over the life of the | ||
| /// process. It may also no longer refer to this thread at all, since any | ||
| /// thread but the current one can exit at any point. For anything other than | ||
| /// the current thread, logging is the only safe use. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's usable for any thread that is running, not just the current right? I think the property to convey is that OS TIDs uniquely represent a thread among other running threads, which effectively means that if a thread isn't known to be running then ID can only be used in cases where non-uniqueness is okay (e.g. logging). And then one way to know the thread is running is if you're looking at the current thread's ID. Not sure how best to put this into words.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So there are conditions under which it is safe to use os_id and it refers to correct thread, however in other conditions it should be used only for cases when stale os_id reference is harmless, like logging. I have tried to reword this section to better communicate this, thank. Let me know if you see any better way to put this into words. |
||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// #![feature(thread_os_id)] | ||
| /// use std::thread; | ||
| /// | ||
| /// let spawned = thread::spawn(|| thread::current().os_id()); | ||
| /// println!("spawned thread ran as {:?}", spawned.join().unwrap()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this do the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, applied the changes but guarded it so it will not fail on other platforms that do not support os_id and could return None making it fail. |
||
| /// ``` | ||
| #[unstable(feature = "thread_os_id", issue = "160215")] | ||
| #[must_use] | ||
| pub fn os_id(&self) -> Option<u64> { | ||
| self.inner.os_id.get() | ||
| } | ||
|
|
||
| /// Gets the thread's name. | ||
| /// | ||
| /// For more information about named threads, see | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Why doesn't
Thread::newjust callset_os_id_to_currentinternally?If there are callers where this doesn't work, maybe we should have two constructors?
Thread::new_currentuses current OS idThread::new_remotetakes OS id as paramterI think this would also avoid the
OnceLock.View changes since the review
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 added Thread::new_current just for the current path, however I think we couldn't get rid of sync primitives for os_id because of the spawn_unchecked path, in that case when we create the Thread, we are still running on parent thread, so using set_os_id_to_current would assign parent os_id to child thread that it is trying to spawn.