Skip to content

process: add libuvHandlesCount()#27145

Closed
XadillaX wants to merge 2 commits into
nodejs:masterfrom
XadillaX:libuv_handle_count
Closed

process: add libuvHandlesCount()#27145
XadillaX wants to merge 2 commits into
nodejs:masterfrom
XadillaX:libuv_handle_count

Conversation

@XadillaX

@XadillaX XadillaX commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

This method is to count libuv handles of current tick.

Checklist
  • make -j4 test (UNIX) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This method is to count libuv handles of current tick.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 9, 2019
@joyeecheung

Copy link
Copy Markdown
Member

If we are going to add a method that returns the handles, I think a better place would be perf_hooks since that's where we have been adding similar statistics.

};
}

process.LIBUV_HANDLE_TYPES = {

@joyeecheung joyeecheung Apr 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not hard code this in JS land, this can be attached in the binding - e.g. see require('os').constants

@XadillaX

XadillaX commented Apr 9, 2019

Copy link
Copy Markdown
Contributor Author

@joyeecheung it's not returning the handles but handles count.

@XadillaX XadillaX added the process Issues and PRs related to the process subsystem. label Apr 9, 2019
@mscdex

mscdex commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

What's the use case for this? If it's to find what's keeping a process alive, don't we have user-friendly ways of determining that nowadays, such as with the built-in reporting functionality or something similar?

@sam-github

Copy link
Copy Markdown
Contributor

Wouldn't it be better to return info about the handles in an array, and if anyone wants a count, they can get the array length, or count handles of a particular type or status in the array?

@addaleax

addaleax commented Apr 9, 2019

Copy link
Copy Markdown
Member

Fwiw, @sam-github’s suggestion sounds very close to process._getActiveHandles(). We’ve had some discussion around making that an “official” API for a while, e.g. in #21453. The major differences would be that:

  • We do not track all our handles in that currently, only the ones that correspond to JS wrappers. I’d like to change that, but it would require some low-level refactoring.
  • We do not track handles that are not created by Node.js, e.g. by addons or embedders. I think that’s okay.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 9, 2019
@richardlau

Copy link
Copy Markdown
Member

Use case(s) would be helpful. You can get the handles via report:

const handles = JSON.parse(process.report.getReport()).libuv
const count = Object.keys(handles).length - 1 // -1 accounts for synthetic `loop` type

@Fishrock123

Copy link
Copy Markdown
Contributor

I think my proposed idea if "getActiveResources" covers this better. See #21453

(I would like to get back to that, but not quite sure when...)

@joyeecheung

joyeecheung commented Apr 9, 2019

Copy link
Copy Markdown
Member

it's not returning the handles but handles count.

@XadillaX um, I am confused, does that have anything to do with either of my previous comments?

EDIT: oh, right, I didn't include the word count in my first comment, sorry. But the comments still apply whether it's returning counts or not.

@XadillaX

XadillaX commented Apr 10, 2019

Copy link
Copy Markdown
Contributor Author

What's the use case for this? If it's to find what's keeping a process alive, don't we have user-friendly ways of determining that nowadays, such as with the built-in reporting functionality or something similar?

We may use the count to draw some health chart.

@mscdex

mscdex commented Apr 10, 2019

Copy link
Copy Markdown
Contributor

I don't understand how a handle count has any bearing on process "health"? If anything, process "health" would be more a measure of things like event loop pauses, memory/cpu usage, etc.

But anyway, it sounds like this would be better solved by the built-in process reporting feature instead.

@XadillaX

Copy link
Copy Markdown
Contributor Author

So shall I continue working for this PR? or any suggestions?

@szmarczak

szmarczak commented Sep 15, 2019

Copy link
Copy Markdown
Member

@XadillaX I'm looking forward to this PR. @Fishrock123 probably had forgotten about his PR.

@Fishrock123

Copy link
Copy Markdown
Contributor

I have not forgotten about mine, but some things have changed and I don't have any real reason to see it through.

I still think this not the correct approach.

@szmarczak

Copy link
Copy Markdown
Member

I have not forgotten about mine, but some things have changed and I don't have any real reason to see it through.

If only I had known that before :P Okay, I'll work on yours then. It doesn't seem like there's much work to do.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell

jasnell commented Jun 25, 2020

Copy link
Copy Markdown
Member

This unfortunately has stalled out and has not seen any further activity. Closing but it can be reopened if it is picked back up again and rebased

@jasnell jasnell closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants