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

Incorrectly detects unref'ed Workers #59

Closed
RaisinTen opened this issue Apr 16, 2022 · 1 comment · Fixed by #60
Closed

Incorrectly detects unref'ed Workers #59

RaisinTen opened this issue Apr 16, 2022 · 1 comment · Fixed by #60

Comments

@RaisinTen
Copy link
Contributor

RaisinTen commented Apr 16, 2022 •
edited
Loading

The logs include WORKER which has been unref'ed already and doesn't keep node running.

test.js

const log = require('why-is-node-running');
new (require('worker_threads').Worker)('', { eval: true }).unref();
log();

output

$ node test.js
There are 10 handle(s) keeping the process running

# WORKER
node:internal/async_hooks:201
node:internal/worker:185
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981
node:internal/modules/cjs/loader:822
node:internal/modules/run_main:77
node:internal/main/run_main_module:17

# MESSAGEPORT
node:internal/async_hooks:201
node:internal/worker:185
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981
node:internal/modules/cjs/loader:822
node:internal/modules/run_main:77
node:internal/main/run_main_module:17

# TTYWRAP
node:internal/async_hooks:201
node:internal/bootstrap/switches/is_main_thread:47
node:internal/bootstrap/switches/is_main_thread:127
node:internal/worker:213
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981

# SIGNALWRAP
node:internal/async_hooks:201
node:internal/process/signal:26
node:internal/bootstrap/switches/is_main_thread:132
node:internal/worker:213
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103

# TickObject
node:internal/async_hooks:201
node:internal/async_hooks:506
node:internal/process/task_queues:133
node:internal/streams/readable:980
node:internal/streams/readable:971
node:internal/streams/readable:885
node:internal/streams/readable:751
node:internal/worker:426
node:internal/worker:213

# TTYWRAP
node:internal/async_hooks:201
node:internal/bootstrap/switches/is_main_thread:47
node:internal/bootstrap/switches/is_main_thread:139
node:internal/worker:218
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981

# TickObject
node:internal/async_hooks:201
node:internal/async_hooks:506
node:internal/process/task_queues:133
node:internal/streams/readable:980
node:internal/streams/readable:971
node:internal/streams/readable:885
node:internal/streams/readable:751
node:internal/worker:426
node:internal/worker:218

# MESSAGEPORT
node:internal/async_hooks:201
node:internal/worker:223
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981
node:internal/modules/cjs/loader:822
node:internal/modules/run_main:77
node:internal/main/run_main_module:17

# MESSAGEPORT
node:internal/async_hooks:201
node:internal/worker:223
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981
node:internal/modules/cjs/loader:822
node:internal/modules/run_main:77
node:internal/main/run_main_module:17

# TickObject
node:internal/async_hooks:201
node:internal/async_hooks:506
node:internal/process/task_queues:133
node:internal/worker:261
/Users/raisinten/Desktop/temp/project/npm/test.js:2 - new (require('worker_threads').Worker)('', { eval: true }).unref();
node:internal/modules/cjs/loader:1103
node:internal/modules/cjs/loader:1157
node:internal/modules/cjs/loader:981
node:internal/modules/cjs/loader:822

I'll try to open a PR in Node.js to implement the hasRef() method on Worker and if that gets merged, the only change required in this repository would be to additionally handle the 'WORKER' type in

why-is-node-running/index.js

Line 30 in 24fb4c8

r.type === 'Timeout' &&
.

EDIT:
PR opened in Node.js - nodejs/node#42756.
RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 16, 2022
@RaisinTen
worker: add hasRef()
68380b6 
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 RaisinTen mentioned this issue Apr 16, 2022
Merged
RaisinTen added a commit to RaisinTen/node that referenced this issue Apr 16, 2022
@RaisinTen
worker: add hasRef()
3a000eb 
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
@RaisinTen
worker: add hasRef() to the handle object
abbe313 
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 to nodejs/node that referenced this issue Apr 19, 2022
@RaisinTen
worker: add hasRef() to the handle object
1b8d179 
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>
@RaisinTen RaisinTen mentioned this issue Apr 22, 2022
Closed
RaisinTen added a commit to RaisinTen/why-is-node-running that referenced this issue Apr 24, 2022
@RaisinTen
Correctly detect unref'ed handles
4e1788d 
At this point, Timeout objects aren't the only handles that have a
hasRef() method. nodejs/node#42756 added
hasRef() to the Worker handle object that gets reported by async_hooks,
so we should consider that too.

Fixes: mafintosh#59
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen mentioned this issue Apr 24, 2022
Merged
@RaisinTen
Copy link
Contributor Author

Fix: #60

xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
@RaisinTen @xtx1130
worker: add hasRef() to the handle object
2e16a4e 
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>
mafintosh pushed a commit that referenced this issue Apr 26, 2022
@RaisinTen @mafintosh
Correctly detect unref'ed handles
48c2e21 
At this point, Timeout objects aren't the only handles that have a
hasRef() method. nodejs/node#42756 added
hasRef() to the Worker handle object that gets reported by async_hooks,
so we should consider that too.

Fixes: #59
Signed-off-by: Darshan Sen <raisinten@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Apr 28, 2022
@RaisinTen @targos
worker: add hasRef() to the handle object
c3922af 
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 to nodejs/node that referenced this issue May 31, 2022
@RaisinTen @juanarbol
worker: add hasRef() to the handle object
2fe8a79 
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>
danielleadams pushed a commit to nodejs/node that referenced this issue Jun 27, 2022
@RaisinTen @danielleadams
worker: add hasRef() to the handle object
51a0fd1 
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 to nodejs/node that referenced this issue Jul 12, 2022
@RaisinTen @targos
worker: add hasRef() to the handle object
1f761a4 
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 to nodejs/node that referenced this issue Jul 31, 2022
@RaisinTen @targos
worker: add hasRef() to the handle object
293bab7 
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>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
@RaisinTen @guangwong
worker: add hasRef() to the handle object
952e0e2 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Correctly detect unref'ed handles RaisinTen/why-is-node-running
1 participant
@RaisinTen