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

Add hasRef to worker_threads.Worker #42091

Closed
SimenB opened this issue Feb 23, 2022 · 17 comments · Fixed by #42756
Closed

Add hasRef to worker_threads.Worker #42091

SimenB opened this issue Feb 23, 2022 · 17 comments · Fixed by #42756
Labels
feature request Issues that request new features to be added to Node.js. worker Issues and PRs related to Worker support.

Comments

@SimenB
Copy link
Member

SimenB commented Feb 23, 2022

What is the problem this feature will solve?

Jest has a --detect-open-handles flag which attempts to figure out (using async_hooks) what resources (timer/server etc.) are preventing a test run/node from exiting. To avoid false positives we perform filtering before presenting the list to the user. One of those things is to check if a Timer has been unrefed or not, via Timer.hasRef.

However, Workers have no hasRef, even though they have the {un}ref pair, so Jest will currently print false positives for Workers that have had unref called.

What is the feature you are proposing to solve the problem?

Add Worker.hasRef similar to the Timer API.

What alternatives have you considered?

No response

@SimenB SimenB added the feature request Issues that request new features to be added to Node.js. label Feb 23, 2022
@targos
Copy link
Member

targos commented Feb 23, 2022

What about other APIs that have an unref function? ChildProcess, FSWatcher, etc.

@SimenB
Copy link
Member Author

SimenB commented Feb 23, 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.

@VoltrexKeyva VoltrexKeyva added the worker Issues and PRs related to Worker support. label Feb 23, 2022
@benjamingr
Copy link
Member

@SimenB would you be interested in contributing a PR? I am happy to guide you, the code is in io.js and worker.js mostly :)

@SimenB
Copy link
Member Author

SimenB commented Feb 23, 2022

Io.js, that's a blast from the past! Happy to contribute it if it'll get accepted 🙂

daeyeon added a commit to daeyeon/node that referenced this issue Apr 14, 2022
Refs: nodejs#42091

The issue above reported a specific use case about `Timer.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)
daeyeon added a commit to daeyeon/node that referenced this issue Apr 14, 2022
Refs: nodejs#42091

The issue above reported a specific use case about `Timer.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)
daeyeon added a commit to daeyeon/node that referenced this issue Apr 14, 2022
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)
RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 16, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: nodejs#42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor

Sent a PR for this: #42756

RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 16, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: nodejs#42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 17, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: nodejs#42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Apr 19, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@SimenB
Copy link
Member Author

SimenB commented Apr 21, 2022

I tried this in the nightly now (19.0.0-nightly20220421c65746e05b) which I believe has the above fix. Jest now reports MESSAGEPORT instead of Worker, but it still reports something. Should MESSAGEPORT be ignored?

@RaisinTen
Copy link
Contributor

@SimenB I'm assuming that happens after you merged jestjs/jest#12705?

That happens because the unref() method on the Worker object doesn't synchronously unref the associated MessagePorts. You may create a new issue for this.

If you ignore the MESSAGEPORT handles, i.e., bring back if (type === 'Timeout' || type === 'Immediate') { and add WORKER to the list, it would also ignore legit MESSAGEPORT handles that are open, like in

const { MessageChannel } = require('worker_threads');
const { port1 } = new MessageChannel();
port1.on('message', () => {}); // You would need to call `port1.unref()` to stop the event loop.

But I guess Jest users won't even notice it because --detect-open-handles never reported MESSAGEPORT handles in the first place. So I think MESSAGEPORT handles can be ignored for now and you may add WORKER to the list.

@SimenB
Copy link
Member Author

SimenB commented Apr 22, 2022

After that, yes 👍 would waiting for some time help the message channel be closed?

@RaisinTen
Copy link
Contributor

would waiting for some time help the message channel be closed?

nope, it just stays open forever unless you do something to close it

@SimenB
Copy link
Member Author

SimenB commented Apr 22, 2022

Aha. But if it doesn't keep the process open, I think we can ignore it? Or would your example of new MessagePort keep the process open? I'm currently AFK, so can't test this myself

@RaisinTen
Copy link
Contributor

Actually it does keep the process running.

@SimenB
Copy link
Member Author

SimenB commented Apr 22, 2022

Oh 😅 how would you recommend we deal with this?

@RaisinTen
Copy link
Contributor

We have 2 options here:

  1. if we ignore messageport handles for now until someone comes up with a fix, jest users won't notice it because the bug was there from the beginning
  2. or we can still report such false positives but the advantage is that in case the process keeps running, the logs would always include the root cause, which might not happen if we just form an allowlist of timer and worker handles in the report

I would recommend option 1 if the feature is not used very much and option 2 if it is used a lot. :P
I think https://github.com/mafintosh/why-is-node-running uses option 2, which is why I reported mafintosh/why-is-node-running#59.

@RaisinTen
Copy link
Contributor

if we ignore messageport handles for now until someone comes up with a fix, jest users won't notice it because the bug was there from the beginning

Sorry, I misunderstood the default behavior. Jest reports handles as open by default, so excluding MessagePort would definitely be noticeable. So scratch option 1, option 2 is better IMO.

RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 24, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: nodejs#42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor

@SimenB here's the fix for messageports btw - #42849

@SimenB
Copy link
Member Author

SimenB commented Apr 24, 2022

Great, thanks!

xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: nodejs#42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue May 2, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: #42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this issue May 2, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: #42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@SimenB
Copy link
Member Author

SimenB commented May 3, 2022

@RaisinTen can confirm the jest detection works correctly on 18.1.0, thank you!

juanarbol pushed a commit that referenced this issue May 31, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol pushed a commit that referenced this issue May 31, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: #42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this issue Jun 27, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: #42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this issue Jul 31, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: #42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: #42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
This should help projects like
https://github.com/mafintosh/why-is-node-running and
https://github.com/facebook/jest to detect if Worker instances are
keeping the event loop active correctly.

Fixes: nodejs/node#42091
Refs: mafintosh/why-is-node-running#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42756
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
Since we were removing the hasRef() method before exposing the
MessagePort object, the only way of knowing if the handle was keeping
the event loop active was to parse the string returned by
util.inspect(port), which is inconvenient and inconsistent with most of
the other async resources. So this change stops removing hasRef() from
the MessagePort prototype. The reason why this is also being documented
is that while reporting active resources, async_hooks returns the same
MessagePort object as the one that is accessible by users.

Refs: nodejs/node#42091 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants