-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Open
valentynkit
wants to merge
6
commits into
rust-lang:main
Choose a base branch
from
valentynkit:thread-os-id
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+81
−3
Open
Implement Thread::os_id
#160219
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c496571
Implement `Thread::os_id`
valentynkit 84d2f10
Store the OS thread id in a `OnceLock`
valentynkit 3a3ff33
Add `Thread::new_current` for current-thread handles
valentynkit 5c8cb4e
Reword the `os_id` docs after review
valentynkit 42cdf8c
Rename the spawned id binding in the `os_id` test
valentynkit 71be0e9
Address review feedback on `Thread::os_id`
valentynkit File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,42 @@ 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. | ||
| /// | ||
| /// Takes no name because passing one into `Thread::new` allocates with the | ||
| /// global allocator, which `thread::current` is documented never to use. | ||
| pub(crate) fn new_current(id: ThreadId) -> Thread { | ||
| let thread = Thread::new(id, None); | ||
| 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. | ||
| /// | ||
| /// `imp::current_os_id` must not allocate with the global allocator or call | ||
| /// `thread::current`. | ||
| pub(crate) fn set_os_id_to_current(&self) { | ||
| if let Some(os_id) = imp::current_os_id() { | ||
| if self.inner.os_id.set(os_id).is_err() { | ||
| rtabort!("thread OS id already set"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Like the public [`park`], but callable on any handle. This is used to | ||
| /// allow parking in TLS destructors. | ||
| /// | ||
|
|
@@ -204,6 +235,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 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. | ||
| /// | ||
| /// The operating system may reuse the id of a thread that has exited, and a | ||
| /// `Thread` handle can outlive the thread it refers to. Use the id only | ||
| /// where a reused id is harmless, such as logging. | ||
| /// | ||
| /// # 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() | ||
|
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. this will be stale after a fork, and i think it is a reasonable want of an api consumer to wish to be able to use this in conjunction with forking. unsure if there's a nice way to check whether we have forked, though |
||
| } | ||
|
|
||
| /// Gets the thread's name. | ||
| /// | ||
| /// For more information about named threads, see | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The SGX impl of
current_os_idappears to return the address ofthread::current()'s allocatedArc<Thread>if I'm reading it right. I think under the current design, that allocation is not guaranteed to exist and so this will hit the BUSY / re-entrant case inthread::current?Specifically the sequence is:
spawn_uncheckedthread::current()Thread::new_currentimp::current_os_idthread::current()(On the spawn_unchecked path we'd set_current before we hit this code).
I think the two fixes are either (a) we modify
thread::current()to call set_os_id after initializing the thread-local pointer to Arc or (b) we change SGX to have some other implementation (e.g. use the Rust ID).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).
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.
That
thread::current()in SGX isn'tstd::thread::currentsgx.rsimportsthreadfromcrate::sys::pal::abi, so it resolves toabi/thread.rs. So I don't see the path that will hit the BUSY case instd::thread::current.I went through every
current_os_idimpl and none callsstd::thread::current, it may only be true for today, so I've documented it onset_os_id_to_current: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
std::thread::currentinimp::current_os_id, I don't see any need in it beyond future-proofing, however I may be missing something.It also wouldn't cover the
DESTROYEDbranch ofcurrent_or_unnamed, where the handle is a temporary that never goes intoCURRENT.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.
So, on SGX we just get the thread address? I guess that's justifiable but it means that the meaning of
os_idis platform-specific; this thread ID won't show up in a debugger or the likes.Then again I don't know if anyone cares to use debuggers in SGX, so /shrug I wouldn't consider this blocking but it could use a doc note