worker: only unref port for stdin if we ref’ed it before · nodejs/node@2053dd0 · GitHub

nodejs / node Public

• Notifications You must be signed in to change notification settings
• Fork 36.6k
• Star 120k

• Code
• Issues 637
• Pull requests 761
• Actions
• Projects
• Security and quality
• Insights

Additional navigation options

Commit 2053dd0

Browse filesBrowse the repository at this point in the historyBrowse files

addaleax

authored and

targos

committed

worker: only unref port for stdin if we ref’ed it before

We set the `kStartedReading` flag from `_read()` for Worker stdio, and then `ref()` the port. However, the `.on('end')` handler is also attached when `._read()` is not called, e.g. when `process.stdin` inside a Worker is prematurely ended because stdin was not enabled by the parent thread. In that case, we should not call `.unref()` for stdin if we did not also call `.ref()` for it before. Fixes: #28144 PR-URL: #28153 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

1 parent c14e4d5 commit 2053dd0
Copy full SHA for 2053dd0

2 files changed

+24-2Lines changed: 24 additions & 2 deletions

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

• lib/internal/worker
  • io.js
• test/parallel
  • test-worker-no-stdin-stdout-interaction.js

Expand file treeCollapse file tree

Open diff view settings

Collapse file

‎lib/internal/worker/io.js‎

Copy file name to clipboardExpand all lines: lib/internal/worker/io.js

+4-2Lines changed: 4 additions & 2 deletions

Original file line number	Diff line number	Diff line change
@@ -169,8 +169,10 @@ class ReadableWorkerStdio extends Readable {
169	169	this[kIncrementsPortRef] = true;
170	170	this[kStartedReading] = false;
171	171	this.on('end', () => {
172		- if (this[kIncrementsPortRef] && --this[kPort][kWaitingStreams] === 0)
173		- this[kPort].unref();
	172	+ if (this[kStartedReading] && this[kIncrementsPortRef]) {
	173	+ if (--this[kPort][kWaitingStreams] === 0)
	174	+ this[kPort].unref();
	175	+ }
174	176	});
175	177	}
176	178

Collapse file

‎test/parallel/test-worker-no-stdin-stdout-interaction.js‎

Copy file name to clipboard

+20Lines changed: 20 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -0,0 +1,20 @@
	1	+ 'use strict';
	2	+ const common = require('../common');
	3	+ const assert = require('assert');
	4	+ const { Worker, isMainThread } = require('worker_threads');
	5	+
	6	+ // Regression test for https://github.com/nodejs/node/issues/28144.
	7	+
	8	+ if (isMainThread) {
	9	+ const w = new Worker(__filename);
	10	+ w.on('exit', common.mustCall((status) => {
	11	+ assert.strictEqual(status, 0);
	12	+ }));
	13	+ w.stdout.on('data', common.mustCall(10));
	14	+ } else {
	15	+ process.stdin.on('data', () => {});
	16	+
	17	+ for (let i = 0; i < 10; ++i) {
	18	+ process.stdout.write(`processing(${i})\n`, common.mustCall());
	19	+ }
	20	+ }

0 commit comments

Comments

0 (0)