Print thread ID in panic message if thread name is unknown #115746
+16
−9
Conversation
This file contains 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
|
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
I think this needs FCP since it's a visible change @rustbot label -T-libs -T-compiler +T-libs-api |
rustbot
added
T-libs-api
tgross35
force-pushed
the
unnamed-threads-panic-message
branch
from
d68fab6
to
371dd28
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
tgross35
force-pushed
the
unnamed-threads-panic-message
branch
2 times, most recently
from
8e72037
to
3da1046
Compare
rustbot
removed
the
T-compiler
|
Maybe or ? That fits more with the way named threads are shown. |
|
Note that this is a rust-internal thread ID, which isn't the OS thread ID which can be relevant if you're also logging from C code or looking at things with GDB. And |
I just proposed the syntax as a demo, figured libs-api would make the final choice - though between those two, I like the first one better
It is kind of unfortunate that Rust and C won't use the same thread IDs. But this seems worthwhile still - Rust-only applications printing the thread ID in logs is common ( This is all inspired by the below failure I recently had in an highly threaded system. The entire backtrace uselessly displayed one repeated function, with no hints about which preceding log events were relevant: I don't think that stability of the thread ID is much of a concern since we will always have some integer ID for a thread, even if the semantics of that ID might change. The important thing is probably just that this panic message thread ID is the same as what shows up as |
|
Another display option - it looks a bit cleaner to not even call out that a thread is unnamed, and I don't think it adds much Or we could use I also tried to poke some discussion at #67939 |
|
☔ The latest upstream changes (presumably #115627) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@joshtriplett do you have any thoughts on this? |
tgross35
force-pushed
the
unnamed-threads-panic-message
branch
from
3da1046
to
e055544
Compare
|
Sorry to add bike shedding to the thread. Here is a slight derivative of the last suggestion, which omits a "with": Although it's uglier than using the thread identifier in a sentence, using the Debug form makes it look like a Rust value rather than something else. It's also consistent with other output from Rust, although the case can definitely be made that thread IDs deserve a special case. |
|
I know Josh has a pretty busy queue, so perhaps it is best to reroll r? libs-api |
I think it wouldn't need one. T-libs-api FCP is primarily for any time a permanent API commitment is being made by the standard library, and deprecations. In contrast, anything that we can just change back with minimal effort or disruption would not warrant FCP. #112849 had a T-libs FCP but I am not familiar with their criteria for that. @rust-lang/libs would you want to handle this PR? In any case, this looks good to me. |
dtolnay
approved these changes
Dec 1, 2023
|
I don't know that this needs FCP. I'm not opposed and there is precedent for it, but OTOH we wouldn't FCP a panic message change somewhere in the stdlib. So I don't feel strongly. In any case, I think this change is a good idea, and am in favor. |
|
Team member @thomcc has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
Hrm, really? I'd rather be in favor of having the OS ID here since that's what external tools will use. |
I am largely going off of what Really what I think would be best is something like I proposed in #67939 (comment), where our This change shouldn't block something better in the future though. |
|
I think we should also keep the simpler option open to have a separate method to get the os thread ID and later switch to that rather than using the composite u128 id. |
|
Showing the ID for unnamed threads seems useful to tell them apart, but I'd prefer something like |
|
In an attempt to resolve the minor impasse here, I created a poll: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Printing.20thread.20ID.20on.20panic.20bikeshed. All votes welcome |
|
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
cuviper
added
S-waiting-on-fcp
|
☔ The latest upstream changes (presumably #120881) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
|
It would be useful to print the id even for named thread. rustc for example uses OS ids may not be as useful for debug printing as they could be reused (which is the case on Windows). |
|
I don't think that's too much of an issue. IDs will never overlap and it's better than printing the same name for each thread. If it is a concern then also printing the thread creation time would work to disambiguate. |
|
@rfcbot concern ambiguous-non-os-thread-id Ideally, I think this should be displaying an OS thread ID, which is more useful. If we have to display a Rust thread ID instead, and we don't make those match OS thread IDs, then I think we need to unambiguously identify it as such so people don't get surprised when it isn't an OS thread ID. How easily could we use the OS thread ID here? If we can do so easily, I think we should. |
It is somewhat unfortunate that the poll was only posted with the unnamed options and the named options were only added later so imo it's unclear whether it settled the
question |
If we do that then we really should have a way for the user to query the OS thread ID.
Yes. Thread IDs are guaranteed not to conflict (at least while they're active) whereas thread names have no such guarantee. |
Unfortunately, it turns out this isn't very easy for a running thread. Either we need to store the OS TID when we create the thread, or we need to do a bit of a hack to get it from libc via the handle (see the Zulip thread, I also reached out to the glibc maintainers). I think everyone agrees that using the OS TID would be better, but doing that doesn't seem to make sense if we don't also expose that API to the user. It wouldn't be as useful to print OS TID in panic messages if you don't also have a (straightforward) way to print it in log messages. So regarding the concern, my proposal is:
I think most people have been answering yes to this one, and I generally agree |
`panic!` does not print any identifying information for threads that are
unnamed. However, in many cases, the thread ID can be determined.
This changes the panic message from something like this:
thread '<unnamed>' panicked at src/main.rs:3:5:
explicit panic
To something like this:
thread '<unnamed>' (1234) panicked at src/main.rs:3:5:
explicit panic
This change applies to both named and unnamed threads. There is no
change for threads where an ID cannot be determined.
tgross35
force-pushed
the
unnamed-threads-panic-message
branch
from
e055544
to
c322091
Compare
|
I updated this PR to always print the thread ID (as it seems like most people want), but removing the actual text "id" (as was brought up on Zulip): I am happy to change this as needed, but just need a specific suggestion from the voting members to hopefully not bikeshed too much more. I only updated a portion of the UI tests in case there are further changes, will get the rest after FCP starts. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@joshtriplett was the above enough to resolve your concern? I think we just need to go forward assuming we won't have the OS TID for a while, though I do think it makes sense to develop a way to access (separately). So just need libs to indicate a preferred syntax :) |
|
We discussed this in today's @rust-lang/libs meeting. We don't necessarily need a separate way to get the thread ID; the standard library just needs a way to obtain it internally so it can print it. (It can either do this at panic time or use it from thread-local data if we already have it there.) This doesn't have to be perfect; not all platforms will necessarily have this. It would be fine to plumb it through for the tier 1 targets at first (Windows/macOS/Linux), and use non-OS thread IDs for now on other targets until those targets wire it up. (Also, for future reference, there was no objection in the meeting to showing both, iff anyone has a need for the Rust thread ID and they're both clearly identified, though we didn't have an immediate idea of something that'd need the Rust thread ID.) |
|
Speaking for myself and not the team, these formats seemed reasonable to me (where the number is the OS thread ID if available): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
panic!does not print any identifying information for threads that are unnamed. However, in many cases, the thread ID can be determined. This can be useful to tie panic messages to error logs.This changes the panic message from something like this:
To something like this:
There is no change in output for named threads or for threads where an ID cannot be determined.
Unanswered questions
as_u64even though that API may not become stable. I think this is ok since we are not bound to this exact text and can change it in the future if needed. An alternative is to use the debug formatThreadId(2)(which I have seen used by log formatters), but I do not think this really gains much.