Implement Thread::os_id - #160219
Conversation
| 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". |
There was a problem hiding this comment.
Nothing guarantees that the OS's ID is non-zero, we really shouldn't use zero as a sentinel.
There was a problem hiding this comment.
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>.
I'm also not sure how it works out for spawn hooks, which get &Thread on the parent before the thread exists.
You mentioned you have an implementation, so if you've already worked that out I'd rather build on it than guess.
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
|
A meta comment: I appreciate your recent contributions to the project, you are clearly interested in helping out and invest time and thought into it. At the same time, especially the description of this PR makes it very obvious that you are using a LLM to aid you in your contributions, both in writing code and comments. These kinds of cases have been very intensely discussed within the project, and the result of that discussion is our LLM policy that will come into effect next week. Under that policy your LLM usage is deemed forbidden, and failing to declare it (like you have been doing) may result in moderation actions against you. I'm not a moderator, and this is not a moderation warning, just some friendly advice that I want to give you, stemming from years of working on the standard library: In my experience, the hard part about working on the standard library is not actually writing the code, but in
An LLM can do neither those things. While they are admittedly good at doing the mechanical task of writing the code, they are awfully unreliable when it comes to providing evidence, have little to no intrinsic capability to exercise good judgement and are fundamentally just not you. As an example, the current description of this PR is mostly just a very detailed summary of its changes. I can see those myself, thank you very much, that's what the "Files changed" tab is for! The much more interesting questions in this case are e.g. why Thus, please, remember to take time to think and research and be the author, not just the editor, of your communication and your code. If you want to learn how best to use (or not use) LLMs, feel free to join our LLM-mentoring channel on Zulip. As for me, I'm not interested in some stochastic parrot's output, but in other people's, since that's what I learn and thrive from. That's why I invest my time in this project anyway, and am very happy to help other people out if they get stuck. I must insist however that the responsibility of doing the thinking and research doesn't fall on my shoulders alone. @rustbot author |
Thanks for your advice, I totally agree with inappropriate use of LLM in this PR, I usually trying to take more time preparing the PR and understanding all the nuances, this one was bad and too heavily relied on AI. Thanks and I will take it into consideration. As for this PR, I will take some time to really reason about it and also consider other approaches. Sorry for that. :) |
| /// use std::thread; | ||
| /// | ||
| /// let spawned = thread::spawn(|| thread::current().os_id()); | ||
| /// println!("spawned thread ran as {:?}", spawned.join().unwrap()); |
There was a problem hiding this comment.
Could this do the assert_ne! test here? I think that demos relevant properties a bit better.
| /// 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. |
There was a problem hiding this comment.
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.
| /// | ||
| /// 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 or offers no way to read it, |
There was a problem hiding this comment.
Some sandboxes or containers allow per-function filtering, so it is also possible the thread ID exists with an API to read it, but the process can't do that.
| /// it. `None` means the platform has no such id or offers no way to read it, | |
| /// it. `None` means the platform has no such id or it can't be read, |
| let thread = Thread::new(id::get_or_init(), None); | ||
| thread.set_os_id_to_current(); |
There was a problem hiding this comment.
Why doesn't Thread::new just call set_os_id_to_current internally?
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 paramter
I think this would also avoid the OnceLock.
Implements
Thread::os_idas an unstable feature, per the accepted ACP rust-lang/libs-team#635.Tracking issue: #160215
It returns the OS-level thread id, the one that shows up in tools like
psandtop, debuggers, and crash logs.ThreadIdhas no guaranteed relationship to it.Most of the work is already in tree:
sys::thread::current_os_idwas added for the thread id in the panic message. This adds a field to hold the value and the public accessor.Implementation
Per the meeting notes on the ACP,
Threaddoes not store apthread_t, since that goes invalid on join or detach while the handle outlives it. The id is captured while the thread runs instead. The two documentation warnings the notes asked for are on the method.Thread::newhas three callers.init_currentandcurrent_or_unnamedrun on the thread being described, so they fill the id immediately.spawn_uncheckedruns on the spawning thread before the child exists, so the child fills it inThreadInit::initbefore any user code runs. The field is therefore written after the handle already exists and read from other threads, and a handle whose thread has not started yet reportsNone.The field is a
OnceLock<u64>. An atomic would need a sentinel for "not yet known", which nothing guarantees an OS id cannot collide with, and acfg_selectfor the targets without 64-bit atomics.OnceLockneeds neither.Testing
On aarch64-apple-darwin:
./x check library./x test library/std --no-doc --test-args thread_os_id, 3 passed (the two new ones and the existingtest_thread_os_id_not_equal)./x test library/std --doc --test-args os_id, 1 passed./x fmt --allleaves the diff unchangedThe non-Apple arms of
current_os_iddo not compile on the host, so CI is the check for those.r? libs