-
-
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 5 commits
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 |
|---|---|---|
|
|
@@ -4,8 +4,9 @@ use crate::alloc::System; | |
| use crate::ffi::CStr; | ||
| use crate::fmt; | ||
| use crate::pin::Pin; | ||
| use crate::sync::Arc; | ||
| use crate::sync::{Arc, OnceLock}; | ||
| 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. | ||
|
|
@@ -49,6 +50,7 @@ use thread_name_string::ThreadNameString; | |
| struct Inner { | ||
| name: Option<ThreadNameString>, | ||
| id: ThreadId, | ||
| os_id: OnceLock<u64>, | ||
| parker: Parker, | ||
| } | ||
|
|
||
|
|
@@ -103,13 +105,34 @@ 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(OnceLock::new()); | ||
| Parker::new_in_place(&raw mut (*ptr).parker); | ||
| Pin::new_unchecked(arc.assume_init()) | ||
| }; | ||
|
|
||
| Thread { inner } | ||
| } | ||
|
|
||
| /// Creates a handle for the calling thread, recording its OS id. | ||
| /// | ||
| /// `id` must be the `ThreadId` of the calling thread. | ||
| pub(crate) fn new_current(id: ThreadId, name: Option<String>) -> Thread { | ||
| let thread = Thread::new(id, name); | ||
| thread.set_os_id_to_current(); | ||
| thread | ||
| } | ||
|
|
||
| /// 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 |
||
| let _ = self.inner.os_id.set(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. Why do we ignore the return value here? There should never be multiple calls into this code, right? I'd expect we want a
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. Yes, not today. It could happen if the parent-side fill is added later, on platforms with a by-handle query ( Added |
||
| } | ||
| } | ||
|
|
||
| /// Like the public [`park`], but callable on any handle. This is used to | ||
| /// allow parking in TLS destructors. | ||
| /// | ||
|
|
@@ -204,6 +227,38 @@ 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 that shows up in tools like `ps` and `top`, debuggers and | ||
| /// crash logs, unlike [`ThreadId`], which has no guaranteed relationship to | ||
| /// it. `None` means the platform has no such id, the thread has not started | ||
| /// running yet, or the id could not be read. | ||
| /// | ||
| /// Ids are unique among threads running at the same moment, but the | ||
| /// operating system may reuse the id of a thread that has exited, and a | ||
| /// `Thread` handle can outlive the thread it refers to. As long as you know | ||
| /// the thread is running, the id still refers to that thread; for the | ||
| /// current thread you always know. When you do not, use the id only where a | ||
| /// repeated id is harmless, such as logging. | ||
|
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. Do we have an example of why it would be useful to rely on the (weak) uniqueness guarantee given here? It seems simpler to me to just say that this is the thread id provided by the OS (if one was captured), but make no further guarantees about it. I think at least Linux's pid namespaces guarantee (today) that threads in the same program can't overlap OS IDs, but if we do want to say something about uniqueness it seems worth noting that cross-process that guarantee may not be true.
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. The manual pages (or POSIX) only guarantees that thread IDs are unique within a process at a single point in time: Note also that PID namespaces exist, so it is possible to have duplicate PIDs (and I assume thread IDs as well):
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. I cut these (weak) uniqueness guarantee, it doesn't seem worth stating, nor the platform-specific case. |
||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// #![feature(thread_os_id)] | ||
| /// use std::thread; | ||
| /// | ||
| /// let spawned = thread::spawn(|| thread::current().os_id()).join().unwrap(); | ||
| /// if spawned.is_some() { | ||
| /// assert_ne!(spawned, thread::current().os_id()); | ||
| /// } | ||
| /// ``` | ||
| #[unstable(feature = "thread_os_id", issue = "160215")] | ||
| #[must_use] | ||
| pub fn os_id(&self) -> Option<u64> { | ||
| self.inner.os_id.get().copied() | ||
| } | ||
|
|
||
| /// Gets the thread's name. | ||
| /// | ||
| /// For more information about named threads, see | ||
|
|
||
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.
Per the first part of #160776, I think it is never valid to pass a name into this function since that can cause usage of the Global allocator. Looking at the impl here I think we always pass None, so it seems reasonable to delete the name argument entirely (and leave a comment)?
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.
Thanks, added a comment: