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.

Sign up for GitHub

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

Jump to bottom

async_hooks: add getActiveResources #37262

Closed
wants to merge 11 commits into from
Closed

async_hooks: add getActiveResources #37262

wants to merge 11 commits into from

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Feb 7, 2021 •
edited
Loading

This change picks up the changes from the
referenced PR and moves the implementation of
getActiveResources to async_hooks to return a
summary of the resources.

Refs: #21453

@RaisinTen RaisinTen mentioned this pull request Feb 7, 2021
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targos
Copy link
Member

targos commented Feb 7, 2021 •
edited
Loading

See #21102, #21453, #1128 (and probably others) for why they are still not public.

@RaisinTen
Copy link
Contributor Author

@targos Thanks for the links. It seems, we need to move these methods to async_hooks instead. I'll try to do that.

@RaisinTen RaisinTen changed the title process: make _getActiveRequests and _getActiveHandles public async_hooks: add getActiveRequests and getActiveHandles Feb 7, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen
Copy link
Contributor Author

Should I remove process._getActiveRequests and process._getActiveHandles in this PR itself?

@RaisinTen RaisinTen force-pushed the process/make-getActiveRequests-and-getActiveHandles-public branch from ae9f657 to b9ad96f Compare February 7, 2021 15:02
@aduh95
Copy link
Contributor

aduh95 commented Feb 7, 2021

I think you should pick up the changes in #21453 instead, reading other PRs it seems there good reasons to not make those APIs public as is.

Should I remove process._getActiveRequests and process._getActiveHandles in this PR itself?

Probably not, it would make the PR semver-major, it'd be probably better to land this as semver-minor first.

@RaisinTen

This comment has been minimized.

Sign in to view
aduh95
aduh95 reviewed Feb 10, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Is this ready for reviews or still in the works? Do you want to add the wip label or convert the PR to draft?

lib/async_hooks.js Outdated Show resolved Hide resolved
lib/async_hooks.js Outdated Show resolved Hide resolved
lib/timers.js Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor Author

RaisinTen commented Feb 10, 2021 •
edited
Loading

@aduh95 I'm still trying to make Fishrock123's cherry picked commit pass the CI 😄. I'll add the wip label.
I'll commit your suggestions once this passes CI at least.

@RaisinTen RaisinTen added the wip Issues and PRs that are still a work in progress. label Feb 10, 2021
@RaisinTen RaisinTen force-pushed the process/make-getActiveRequests-and-getActiveHandles-public branch 2 times, most recently from 14bf7cf to 8bdd85b Compare February 10, 2021 15:46
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen RaisinTen changed the title async_hooks: add getActiveRequests and getActiveHandles util: add getActiveResources Feb 12, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 13, 2021 •
edited by RaisinTen
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/36094/ (:heavy_check_mark:)

@RaisinTen RaisinTen added review wanted PRs that need reviews. and removed wip Issues and PRs that are still a work in progress. labels Feb 13, 2021
@RaisinTen
Copy link
Contributor Author

Is this ready for reviews or still in the works? Do you want to add the wip label or convert the PR to draft?

@aduh95 I think this is ready for review now. :D

@RaisinTen
Copy link
Contributor Author

I wonder why a failed check of a previous run is linked in the checks section. 👀

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2021

Can you add documentation please?

@RaisinTen
Copy link
Contributor Author

@aduh95 #37261 was recently opened to document the private underlying functions. Maybe that can be modified to document getActiveResources?

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2021 •
edited
Loading

Could you cherry-pick @yashLadha's commit in your branch maybe? We want the documentation to be in the same PR so the REPLACEME tags stay up-to-date.

Adding the semver-major label as I understand this change removes process._getActiveHandles. Feel free to put the semver-minor label if I get this wrong.

aduh95
aduh95 reviewed Feb 19, 2021
View reviewed changes
lib/internal/util.js Outdated
Comment on lines 442 to 443
const handles = process._getActiveHandles();
const reqs = process._getActiveRequests();
Copy link
Contributor

@aduh95 aduh95 Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could you avoid using the global process here and instead load them from internalBinding('process_methods')? If the plan is to remove the undocumented methods in the future, let's avoid to use it in core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. :)
Replaced it.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2021

I appreciate this being picked back up but I would like us to reconsider a bit how this API works. The use case for getActiveResources() has always been to perform a point-in-time check on what async resources are currently active, that has not changed, but I'm not sure there's a need at all for references to the actual resources to be returned by the API. Alternatively, the API could return what is effectively just a summary, listing the types of resources, their number, they async id's, and so forth without exposing the underlying async resources themselves.

@RaisinTen

This comment has been minimized.

Sign in to view
@RaisinTen RaisinTen added the wip Issues and PRs that are still a work in progress. label Apr 19, 2021
@RaisinTen
util: add getActiveResources
fa990cd 
This change picks up the changes from the
referenced PR and moves the implementation of
`getActiveResources` to `util` to return a summary
of the resources.

Refs: nodejs#21453
@RaisinTen RaisinTen force-pushed the process/make-getActiveRequests-and-getActiveHandles-public branch from 8872d7c to fa990cd Compare April 19, 2021 14:24
@benjamingr
Copy link
Member

Why on util? Seems like a pretty generic place even compared to process or async_hooks?

@RaisinTen
Copy link
Contributor Author

@benjamingr I came across this review comment in the referenced PR and thought it would be appropriate to expose it from util:

We can't put every new thing behind a flag, just as we can't put every new thing on process. The uses of this are most closely aligned with async_hooks, even if there are differences in approach. Otherwise, it is at best a utility function that can go into util.

Originally posted by @jasnell in #21453 (comment)

However, I'm open to exposing this from any other module if that's needed. :)

@jasnell
Copy link
Member

jasnell commented Apr 22, 2021

My general preference would be to expose it via async_hooks. If that's not a fit, then util would be my second choice.

@RaisinTen RaisinTen changed the title util: add getActiveResources async_hooks: add getActiveResources Apr 23, 2021
@RaisinTen
Copy link
Contributor Author

@jasnell I have updated the PR to return a summary of the resources instead. Does the API design look okay now?

@jasnell
Copy link
Member

jasnell commented Apr 27, 2021

@RaisinTen ... I guess the thing that I'm struggling with on this PR is whether this is the right approach. Someone could very easily create an async_hook that tracks init and destroy to monitor which async resources are currently active.

@RaisinTen
Copy link
Contributor Author

@jasnell Yes, I agree. Do you think we should aim towards deleting process._getActive* in favour of async_hooks then?

@RaisinTen RaisinTen mentioned this pull request Nov 14, 2021
Merged
@RaisinTen
Copy link
Contributor Author

Closing in favor of #40813

@RaisinTen RaisinTen closed this Nov 25, 2021
@RaisinTen RaisinTen deleted the process/make-getActiveRequests-and-getActiveHandles-public branch November 25, 2021 13:55
@RaisinTen RaisinTen removed review wanted PRs that need reviews. wip Issues and PRs that are still a work in progress. labels Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RaisinTen @nodejs-github-bot @targos @aduh95 @jasnell @benjamingr