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: deprecate _getActive* private APIs in favour of async_hooks #40804

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Nov 13, 2021

The process._getActiveRequests() and process._getActiveHandles()
functions are not needed anymore because the async_hooks module
already provides functionality that helps us to keep a track of the
active resources.

Signed-off-by: Darshan Sen [email protected]

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 13, 2021
@RaisinTen RaisinTen force-pushed the remove-_getActive-functions-in-favour-of-async_hooks branch from 20336e2 to 82fe642 Compare November 13, 2021 14:06
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 13, 2021
@aduh95
Copy link
Contributor

aduh95 commented Nov 13, 2021

We probably need a runtime deprecation first, right?

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Nov 13, 2021

@aduh95

We probably need a runtime deprecation first, right?

I wasn't sure about the deprecation process for private APIs but looking at #22011, I think so

EDIT: Done!

The `process._getActiveRequests()` and `process._getActiveHandles()`
functions are not needed anymore because the `async_hooks` module
already provides functionality that helps us to keep a track of the
active resources.

Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen RaisinTen force-pushed the remove-_getActive-functions-in-favour-of-async_hooks branch from 82fe642 to bcf7c12 Compare November 13, 2021 14:59
@RaisinTen RaisinTen changed the title process: remove _getActive* functions in favour of async_hooks process: deprecate _getActive* private APIs in favour of async_hooks Nov 13, 2021
@mscdex
Copy link
Contributor

mscdex commented Nov 13, 2021

Don't you need to set up various async hooks in advance to be able to achieve the same thing? Additionally as far as I'm aware, using async_hooks incurs overhead (even as soon as you require() it I believe) whereas the private APIs do not. Being able to probe what's keeping the event loop alive at any given moment (live) in production using these APIs is incredibly useful. If anything I think we should move towards making them "public" instead.

Also even though these functions are "private", I have a feeling this could cause a lot of breakage in the ecosystem.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 13, 2021

I agree with @mscdex.

const NUM = 8;
const connections = [];
const clients = [];
let clients_counter = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave the actual test in place since we're not removing these APIs yet.

@RaisinTen
Copy link
Contributor Author

@mscdex

Don't you need to set up various async hooks in advance to be able to achieve the same thing? Additionally as far as I'm aware, using async_hooks incurs overhead (even as soon as you require() it I believe) whereas the private APIs do not. Being able to probe what's keeping the event loop alive at any given moment (live) in production using these APIs is incredibly useful.

Are you talking about the overhead incurred for this hook creation in particular or any require() call in general?

node/lib/async_hooks.js

Lines 258 to 266 in cf56abe

const storageHook = createHook({
init(asyncId, type, triggerAsyncId, resource) {
const currentResource = executionAsyncResource();
// Value of currentResource is always a non null object
for (let i = 0; i < storageList.length; ++i) {
storageList[i]._propagate(resource, currentResource);
}
}
});

If ^, I think we can get rid of that one in #40810.

If anything I think we should move towards making them "public" instead.

Also even though these functions are "private", I have a feeling this could cause a lot of breakage in the ecosystem.

If we make it public, which exact module/object should we expose this from?

@RaisinTen
Copy link
Contributor Author

Still too early to deprecate. There are some valid uses of the underlying handles that hasn't been addressed yet. (see #40813 (comment) for some uses in public GH repos)

@RaisinTen RaisinTen closed this Jan 19, 2022
@RaisinTen RaisinTen deleted the remove-_getActive-functions-in-favour-of-async_hooks branch January 19, 2022 11:22
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++. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants