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

gpotter2 · 2018-06-02T20:28:26Z

Rebased version of #18844

[Original message]

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.

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

mscdex · 2018-06-02T21:03:12Z

If this is going to be a runtime deprecation, you will still need to have the old functions on process, but they will instead throw a proper deprecation error.

gpotter2 · 2018-06-02T21:06:15Z

That was already said on the previous thread, I had done it but lost the commit, will search for it.

Edit: is there no travis unit test on node ?

addaleax · 2018-06-02T23:39:28Z

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

+function getActiveTimers() {
+  const activeHandles = process.getActiveHandles();
+  return activeHandles.filter((handle) => handle instanceof Timer);
+}


It’s hard to tell for me where this function is being used … also, we’re kind of shifting our timers implementation to not use any handles anymore, /cc @apapirovski

Should I remove it ?

jasnell · 2018-06-03T00:39:05Z

doc/api/process.md

@@ -1044,6 +1044,34 @@ a code.
 Specifying a code to [`process.exit(code)`][`process.exit()`] will override any
 previous setting of `process.exitCode`.

+## process.getActiveHandles()


I would really like to take the opportunity now to move these to the async_hooks module or util module and pull them off the process object.

Because I'd prefer not to expand the API surface area of process if we can reasonably avoid it.

It's already there though and people use it. I don't see how it makes more sense to place it on the async_hooks or util modules when the data returned by these functions have to strictly do with what's going on in the process.

async_hooks has to do with hooking into asynchronous events, which is not the same thing as inspecting what is currently happening at any given moment (without hooks).

util is essentially a box full of helper functions that work with whatever input they're given, so that doesn't really fit either.

Maybe you could discuss that in a future PR? I mean, I'm not qualified to answer about this, but I would agree that it's easier to keep the functions where they are.
On the other hand, they were not documented, and could be moved without any further notice

vsemozhetbyt · 2018-06-03T06:57:31Z

doc/api/deprecations.md

@@ -1005,6 +1005,15 @@ accepted by the legacy `url.parse()` API. The mentioned APIs now use the WHATWG
 URL parser that requires strictly valid URLs. Passing an invalid URL is
 deprecated and support will be removed in the future.

+<a id="DEP0110"></a>
+### DEP0110: _getActiveHandles and _getActiveRequests`


Nit: unnecessary backtick at the end.

vsemozhetbyt · 2018-06-03T07:00:21Z

doc/api/deprecations.md

+
+Type: Runtime
+
+As `getActiveHandles()` and `getActiveRequests()` are getting officially


`process.getActiveHandles()` and `process.getActiveRequests()`?

vsemozhetbyt · 2018-06-03T07:01:24Z

doc/api/deprecations.md

@@ -1053,6 +1062,8 @@ deprecated and support will be removed in the future.
 [`os.networkInterfaces`]: os.html#os_os_networkinterfaces
 [`os.tmpdir()`]: os.html#os_os_tmpdir
 [`process.env`]: process.html#process_process_env
+[`process._getActiveHandles`]: process.html#process_getActiveHandles


This and the next links seem unused in the doc.

vsemozhetbyt · 2018-06-03T09:30:23Z

doc/api/deprecations.md


 Type: Runtime

-As `getActiveHandles()` and `getActiveRequests()` are getting officially
+As `process.getActiveHandles()` and `process.getActiveRequests()` are getting officially


Nit: please wrap long lines at 80 characters to prevent doc linting issues.

Fixed ! (also fixed it in lib/bootstrap/node.js)

vsemozhetbyt · 2018-06-03T10:21:14Z

Early CI: https://ci.nodejs.org/job/node-test-pull-request/15235/

gpotter2 · 2018-06-03T13:31:46Z

Update:

Fixed the build & linter
Squashed commits

vsemozhetbyt · 2018-06-03T13:34:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15241/

gpotter2 · 2018-06-03T13:44:57Z

I don't really get why the linter fails on using ` for strings, as it is used everywhere else in the file :/

vsemozhetbyt · 2018-06-03T13:51:55Z

It seems linter allows backticks only for template strings: with variable/expression interpolation or inner line breaks.

vsemozhetbyt · 2018-06-03T13:55:25Z

However, you can use double quotes to avoid escaping if the string contains single quotes:

node/.eslintrc.js

Line 217 in d528781

'quotes': ['error', 'single', { avoidEscape: true }],

gpotter2 · 2018-06-06T09:12:17Z

@vaibhav93 Replaced single quotes with double quotes.

Edit: squashed the commits

bnoordhuis · 2018-06-06T10:11:32Z

doc/api/deprecations.md

+
+Type: Runtime
+
+As `process.getActiveHandles()` and `process.getActiveRequests()` are getting


"are getting" is a bit vague, just say "are."

bnoordhuis · 2018-06-06T10:13:23Z

doc/api/process.md

+* Returns: {Array}
+
+Returns an array containing all handles that are preventing the Node.js process
+from exiting.


The documentation should make it clear that the returned handles don't always correspond 1-to-1 with user-created handles (only when the internal handle has a .owner property that points back to the user handle but I'd leave out that bit.)

gpotter2 · 2018-06-12T10:05:26Z

Rebased against master to fix the conflicts

Make process.getActiveHandles() and make.getActiveRequests() official and deprecate the older process._getActiveHandles() and process._getActiveRequests(). Also add a small documentation

gpotter2 · 2018-06-19T13:29:28Z

Rebased (again) against master to fix conflicts. (wow)

Fishrock123

IMO these APIs should be rethought a bit before exposure, although exposing something like it officially is pretty much what I'm asking for in #20894 (review) & related comments.

My idea would be more like:

Make it part of the async_hooks module for consistency.
Rename it to getActiveResources() and return both handles and requests at the same time.
Make it return a map of asyncId -> resource, so that this can be used to dynamically fetch a resource from an async hook call.
Ensure that all async_hook resources are properly fetch-able in this way.

AndreasMadsen · 2018-06-21T09:10:47Z

Ensure that all async_hook resources are properly fetch-able in this way.

I agree. But be aware that this is a big task, as many async_hooks resource can have uninitialized properties that causes segmentation fault when using console.log().

edit:

Also, if this is for debugging I would suggest we don't add the extra complexity. As it is straight to implement with async_hooks. Yes there is a performance penalty, but that isn't an issue when debugging. If it is for counting the number of handles, as a performance metric then I would suggest we add an API dedicated to that purpose instead of relying on process._getActiveHandles().length.

The resource objects are very internal and undocumented, I'm not a huge fan of exposing them more than they already are if it can be avoided. This is something I would like to fix in async_hooks too. For example, to expose the public Server object rather than the internal Server handle. I know @addaleax feels the same.

Fishrock123 · 2018-06-21T19:56:24Z

But be aware that this is a big task, as many async_hooks resource can have uninitialized properties that causes segmentation fault

We've never had such an issue from _getActiveHandles() or _getActiveRequests(). I think that is only an issue within the init hook, and is unrelated to this API.

Also, if this is for debugging

It is not for debugging. In my case it is for live (potentially production) resource inspection. Overhead must be minimal.

we don't add the extra complexity

It's actually very little as most of the complexity already is present in handle_wrap and req_wrap. See #21453 for a mostly complete prototype.

gpotter2 · 2018-06-21T21:33:17Z

Feel free to close this PR for #21453 anytime

Fishrock123 · 2018-06-21T22:43:54Z

@gpotter2 Thanks for bringing this up. :)

Feel free to add anything you have (use cases, api requirements) in that PR!

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 2, 2018

mscdex added semver-major PRs that contain breaking changes and should be released in the next major version. process Issues and PRs related to the process subsystem. labels Jun 2, 2018

addaleax reviewed Jun 2, 2018

View reviewed changes

jasnell reviewed Jun 3, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 3, 2018

View reviewed changes

vsemozhetbyt added the deprecations Issues and PRs related to deprecations. label Jun 3, 2018

vsemozhetbyt reviewed Jun 3, 2018

View reviewed changes

bnoordhuis reviewed Jun 6, 2018

View reviewed changes

gpotter2 mentioned this pull request Jun 6, 2018

lambda-local does not wait for empty event loop ashiina/lambda-local#128

Closed

process: make getActive{Handles,Requests} official

ebcb830

Make process.getActiveHandles() and make.getActiveRequests() official and deprecate the older process._getActiveHandles() and process._getActiveRequests(). Also add a small documentation

Fishrock123 suggested changes Jun 20, 2018

View reviewed changes

Fishrock123 mentioned this pull request Jun 20, 2018

async_hooks: add currentResource #21313

Closed

4 tasks

Fishrock123 mentioned this pull request Jun 21, 2018

async_hooks: add getActiveResources (prototype) #21453

Closed

4 tasks

Fishrock123 closed this Jun 21, 2018

gpotter2 deleted the process-getactive branch July 15, 2018 11:09

mscdex mentioned this pull request Nov 8, 2020

doc: acknowledge process._getActiveHandles() and process._getActiveRequests() #36036

Closed

targos mentioned this pull request Feb 7, 2021

async_hooks: add getActiveResources #37262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gpotter2 commented Jun 2, 2018 •

edited

Loading

mscdex commented Jun 2, 2018

gpotter2 commented Jun 2, 2018 •

edited

Loading

addaleax Jun 2, 2018

gpotter2 Jun 3, 2018

jasnell Jun 3, 2018

mscdex Jun 3, 2018

jasnell Jun 3, 2018

mscdex Jun 3, 2018 •

edited

Loading

gpotter2 Jun 6, 2018

vsemozhetbyt Jun 3, 2018

vsemozhetbyt Jun 3, 2018

vsemozhetbyt Jun 3, 2018

vsemozhetbyt Jun 3, 2018

gpotter2 Jun 3, 2018 •

edited

Loading

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 3, 2018 •

edited

Loading

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 3, 2018 •

edited

Loading

vsemozhetbyt commented Jun 3, 2018 •

edited

Loading

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 6, 2018 •

edited

Loading

bnoordhuis Jun 6, 2018

bnoordhuis Jun 6, 2018

gpotter2 Jun 6, 2018

gpotter2 commented Jun 12, 2018

gpotter2 commented Jun 19, 2018

Fishrock123 left a comment •

edited

Loading

AndreasMadsen commented Jun 21, 2018 •

edited

Loading

Fishrock123 commented Jun 21, 2018 •

edited

Loading

gpotter2 commented Jun 21, 2018

Fishrock123 commented Jun 21, 2018


		Type: Runtime

		As `getActiveHandles()` and `getActiveRequests()` are getting officially


		Type: Runtime

		As `process.getActiveHandles()` and `process.getActiveRequests()` are getting

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

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

Conversation

gpotter2 commented Jun 2, 2018 • edited Loading

mscdex commented Jun 2, 2018

gpotter2 commented Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpotter2 Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 3, 2018 • edited Loading

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 3, 2018 • edited Loading

vsemozhetbyt commented Jun 3, 2018 • edited Loading

vsemozhetbyt commented Jun 3, 2018

gpotter2 commented Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpotter2 commented Jun 12, 2018

gpotter2 commented Jun 19, 2018

Fishrock123 left a comment • edited Loading

Choose a reason for hiding this comment

AndreasMadsen commented Jun 21, 2018 • edited Loading

Fishrock123 commented Jun 21, 2018 • edited Loading

gpotter2 commented Jun 21, 2018

Fishrock123 commented Jun 21, 2018

gpotter2 commented Jun 2, 2018 •

edited

Loading

gpotter2 commented Jun 2, 2018 •

edited

Loading

mscdex Jun 3, 2018 •

edited

Loading

gpotter2 Jun 3, 2018 •

edited

Loading

gpotter2 commented Jun 3, 2018 •

edited

Loading

gpotter2 commented Jun 3, 2018 •

edited

Loading

vsemozhetbyt commented Jun 3, 2018 •

edited

Loading

gpotter2 commented Jun 6, 2018 •

edited

Loading

Fishrock123 left a comment •

edited

Loading

AndreasMadsen commented Jun 21, 2018 •

edited

Loading

Fishrock123 commented Jun 21, 2018 •

edited

Loading