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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@addaleax
Copy link
Member

commented Jun 10, 2019

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
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

@addaleax addaleax added the worker label Jun 10, 2019

@nodejs-github-bot

This comment was marked as outdated.

Copy link

commented Jun 10, 2019

Sadly, an error occurred when I tried to trigger a build. :(

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@Trott

Trott approved these changes Jun 10, 2019

chjj added a commit to chjj/bthreads that referenced this pull request Jun 11, 2019

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Landed in d5cf51d

@Trott Trott closed this Jun 20, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jun 20, 2019

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: nodejs#28144
PR-URL: nodejs#28153
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@richardlau

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

This failed in Travis for a new PR (#28327):

https://travis-ci.com/nodejs/node/jobs/209729625#L265-L296

=== release test-worker-no-stdin-stdout-interaction ===
Path: parallel/test-worker-no-stdin-stdout-interaction
--- stderr ---
assert.js:89
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
1 !== 0
    at Worker.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-worker-no-stdin-stdout-interaction.js:11:12)
    at Worker.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:374:15)
    at Worker.emit (events.js:200:13)
    at Worker.[kOnExit] (internal/worker.js:165:10)
    at Worker.<computed>.onexit (internal/worker.js:116:51) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}
--- stdout ---
processing(0)
processing(1)
processing(2)
processing(3)
Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-worker-no-stdin-stdout-interaction.js
===
=== 1 tests failed
===

@addaleax addaleax deleted the addaleax:worker-stdin-stdout branch Jun 20, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@richardlau Since we added all that fancy stages + caching stuff to Travis, having newly added tests fail is something that happens from time to time (presumably because the wrong, old binary is cached or something), so I wouldn’t worry about it unless it turns up repeatedly…

targos added a commit that referenced this pull request Jul 2, 2019

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>

targos added a commit that referenced this pull request Jul 2, 2019

2019-07-03, Version 12.6.0 (Current)
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.0. #28449
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153

PR-URL: #28508

@targos targos referenced this pull request Jul 2, 2019

Merged

Release v12.6.0 proposal #28508

targos added a commit that referenced this pull request Jul 2, 2019

2019-07-03, Version 12.6.0 (Current)
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.0. #28449
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508

targos added a commit that referenced this pull request Jul 2, 2019

2019-07-03, Version 12.6.0 (Current)
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.1. #28449,
    #28511
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508

targos added a commit that referenced this pull request Jul 3, 2019

2019-07-03, Version 12.6.0 (Current)
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.1. #28449,
    #28511
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508
@JoshCheek

This comment has been minimized.

Copy link

commented Jul 3, 2019

🙌 ty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.