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

child_process: add hasRef to ChildProcess #42731

Closed
wants to merge 6 commits into from

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Apr 14, 2022

Refs: #42091

The issue above reported a specific use case about hasRef().
After reading the issue, I started thinking users may naturally expect
hasRef method if ref() and unref() exist on an object they use.
As of now, however, hasRef() exists on Timer only.

This commit suggests adding hasRef method to ChildProcess first.
There are more similar cases. (fs.FSWatcher, fs.StatWatcher, net.Socket,
and so on)

Refs: nodejs#42091

The issue above reported a specific use case about `hasRef()`.
After reading the issue, I started thinking users may naturally expect
`hasRef` method if `ref()` and `unref()` exist on an object they use.
As of now, however, `hasRef()` exists on `Timer` only.

This commit suggests adding `hasRef` method to `ChildProcess` first.
There are more similar cases. (fs.FSWatcher, fs.StatWatcher, net.Socket,
and so on)
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Apr 14, 2022
@daeyeon daeyeon force-pushed the master.node-220414.Thu.4e12 branch from 24eff20 to 9a5f852 Compare April 14, 2022 14:39
This removes assertions where the child could be {un}referred.
@daeyeon daeyeon force-pushed the master.node-220414.Thu.4e12 branch from 9a5f852 to 3cd3796 Compare April 14, 2022 15:47
@@ -519,6 +519,11 @@ ChildProcess.prototype.unref = function() {
if (this._handle) this._handle.unref();
};


ChildProcess.prototype.hasRef = function() {
return this._handle ? this._handle.hasRef() : false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return this._handle ? this._handle.hasRef() : false;
return !!this._handle?.hasRef()

Copy link
Member Author

Choose a reason for hiding this comment

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

applied the suggestion.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2022
@RaisinTen
Copy link
Contributor

cc @nodejs/child_process

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Actually, I don't think we should expose hasRef() from the ChildProcess JS object because jest uses async_hooks to collect open handles and the init() callback gets called with resource set to the underlying PROCESSWRAP handle object (this._handle) which already contains the hasRef() method.

To be able to correctly report open ChildProcess handles correctly, jest needs to change https://github.com/facebook/jest/blob/defbc0535a46119d4682f97bf8a13b5562c1445b/packages/jest-core/src/collectHandles.ts#L96:

  • to also include PROCESSWRAP OR
  • just accept all types of async resources

The reason why Timer objects have the hasRef() method is that it isn't backed up by a C++ HandleWrap instance like the other cases. Essentially, the resource variable that the async_hooks init() callback gets called with is the same as the return value of setTimeout().

@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 17, 2022
@daeyeon
Copy link
Member Author

daeyeon commented Apr 18, 2022

I haven't gotten a bug report in Jest about them, so I have no opinion.
On a more serious note, I think it makes sense for ref, unref and hasRef to always exist together.

In the issue above, although SimenB had no opinion requiring ChildProcess.hasRef() for Jest, I actually paid more attention to his next words regarding consistency across APIs. But now I understand that exposing ref()/unref() pair is common and Timeout.hasRef() is an exceptional case. Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants