-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
src: use node- prefix on thread names
#61307
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?
Conversation
|
Review requested:
|
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61307 +/- ##
==========================================
- Coverage 88.53% 88.51% -0.02%
==========================================
Files 704 704
Lines 208759 208739 -20
Branches 40281 40268 -13
==========================================
- Hits 184816 184762 -54
- Misses 15947 15972 +25
- Partials 7996 8005 +9
🚀 New features to boost your workflow:
|
mcollina
left 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.
lgtm
|
cc @nodejs/diagnostics @sxa You'll need to fix the expected thread name in the failing test. |
Signed-off-by: Stewart X Addison <sxa@ibm.com>
Argh! Not sure why that didn't show up in my testing. I've got two options for the
|
|
Is there any specific reason to keep the same structure for the rest of the names? If we're going to rename might as well rename fully to fit within those constraints. |
Signed-off-by: Stewart X Addison <sxa@ibm.com>
|
Latest commit reverts all but |


This was spotted while doing some other work - the changes made in #56416 (and there were further updates in #59601) mean that when looking at the
topoutput the node process just show as a rather vagueMainThread. To the average user this is likely to be a little confusing as to which process it is and potentially lead people to have concerns about a seemingly rogue named process on their machines (Pressingcwithin top clarifies it, but I'm looking at the general simple use case).As mentioned in the screenshots included in the original PR,
htopdoes not show the main thread name by default so this may have not been spotted in the original testing.This PR adjusts the threadnames to have a
node-prefix so that the output fromtopshows something more meaningful by default - specificallynode-MainThread.Other options considered:
nodejs-MainThreadbut by default the text in thetopoutput got truncated atnodejs-MainThrewhich isn't ideal. I also considerednode (main)as an alternative option.topoutput isn't sensible, but since it's probably the most widely used tool for this sort of thing I think it's best to improve how we look there.I've included a screenshot showing what each option looks like in the
topoutput shown below. Process 930775 is the current main code, 583671 is with this PR, 467427 and 587572 are the other ones from option 1 above.