Skip to content
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

process: make getActive{Handles,Requests} official #18844

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 18, 2018

process._getActiveHandles() and process._getActiveRequests() have been around for a long time, and are utilized to a large enough degree that it makes sense to make them official
APIs.

If this PR is accepted, I'll submit a follow up that deprecates the underscore prefixed versions.

Fixes: #1128

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

process

process._getActiveHandles() and process._getActiveRequests()
have been around for a long time, and utilized to a large
enough degree that it makes sense to make them official
APIs.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 18, 2018
@addaleax
Copy link
Member

Generally +1 to this.

One thing that’s worth noting is that getActiveHandles() reads the .owner property and returns that instead, when it is available. We should probably provide that for all types of HandleWraps?

And similarly for requests, we may want to make sure that what we return is actually public API…

@vsemozhetbyt vsemozhetbyt added the process Issues and PRs related to the process subsystem. label Feb 18, 2018
@bnoordhuis
Copy link
Member

Random observation: there's no .owner for the big TimerWrap handle because it isn't owned by a single timer. There may be more oddballs.


```js
console.log(`Active handles: ${util.inspect(process.getActiveHandles())}`);
```
Copy link
Member

Choose a reason for hiding this comment

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

More information about the things that are in the array would be helpful. What kind of objects are they? What can the user basically do with them, ec

Copy link
Member

Choose a reason for hiding this comment

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

+1, util.inspecting them and then console.loging does not seem to be a good example since console.log will util.inspect them anyway..

Copy link

@ShanMadane ShanMadane Jan 28, 2020

Choose a reason for hiding this comment

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

I would like to know which active handles are open for a particular process id. How can I get it

@@ -298,6 +298,8 @@

function setupProcessObject() {
process._setupProcessObject(pushValueToArray);
process._getActiveHandles = process.getActiveHandles;
process._getActiveRequests = process.getActiveRequests;
Copy link
Member

Choose a reason for hiding this comment

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

Should this include some form of deprecation for the _-prefixed versions?

@addaleax
Copy link
Member

Random observation: there's no .owner for the big TimerWrap handle because it isn't owned by a single timer. There may be more oddballs.

I think we should put in the work to return the actual Timer instances for these.

@cjihrig Btw, I know I'm asking for a lot of things in this PR -- I'd totally be willing to help make those happen, if you want help with anything.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

I'm wondering also if these should not be moved to the async_hooks module? The reason is that, while they are not tied to async_hooks, there is a close enough relationship to align them.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

Ping @cjihrig

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Ping @cjihrig again

@BridgeAR
Copy link
Member

Closing due to no response. @cjihrig please reopen or open a new PR when you plan on working on this again.

@BridgeAR BridgeAR closed this Apr 28, 2018
@gpotter2
Copy link

gpotter2 commented May 29, 2018

Hello! Thanks for NodeJS, it’s an incredible language.

This feature would be nice, so I’m kindly asking if by any chance tho PR could be revised, fixed and merged by a good soul.

I was willing to use the function, but my spider sense always freaks out in front of _ undocumented functions 😃

If this PR is dead, would they be any way to get it back on ? Is there any way one could contribute ? I would love to help but never contributed to NodeJS before, it might be more painful to help me get it right =(

Thanks !

@BridgeAR
Copy link
Member

@gpotter2 feel free to give it a try. You could go ahead and cherry-pick this commit and then add your code on top of that. You'll get feedback and help how to get there. Just ask for things explicitly if you need any help.

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++. lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: a way to inspect what's in the event loop
10 participants