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 threads get killed if they listen for data on stdin #28144

Closed
JoshCheek opened this issue Jun 9, 2019 · 1 comment
Closed

Worker threads get killed if they listen for data on stdin #28144

JoshCheek opened this issue Jun 9, 2019 · 1 comment
Assignees
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.

Comments

@JoshCheek
Copy link

Description

When a worker thread tries to listen for data on stdin, it gets killed.
Not even finally blocks get called.

Environment

  • Version: v12.0.0
  • Platform: Darwin Joshs-MacBook-Air.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64
  • Subsystem: Probably worker_thread

Example

A worker is started, it prints "processing" 10 times:

const { Worker, isMainThread } = require('worker_threads')
if(isMainThread) {
  new Worker(__filename).on('exit', (exitStatus) => console.log({ exitStatus }))
} else {
  // process.stdin.on('data', console.log) // <-- this line is commented out
  for(i=0; i < 10; ++i) console.log(`processing(${i})`)
}
// => processing(0)
// => processing(1)
// => processing(2)
// => processing(3)
// => processing(4)
// => processing(5)
// => processing(6)
// => processing(7)
// => processing(8)
// => processing(9)
// => { exitStatus: 0 }

Now we uncomment the line that listens for data, it gets killed after 2 or 3 iterations (on my machine, guessing quicker on faster machines):

const { Worker, isMainThread } = require('worker_threads')
if(isMainThread) {
  new Worker(__filename).on('exit', (exitStatus) => console.log({ exitStatus }))
} else {
  process.stdin.on('data', console.log) // <-- now this line runs
  for(i=0; i < 10; ++i) console.log(`processing(${i})`)
}
// => processing(0)
// => processing(1)
// => processing(2)
// => { exitStatus: 0 }

Expected Behaviour

  • I expected the second example to behave the same as the first example. Which is to say that I expect my worker would be able to process my stdin.
  • However, if this behaviour is intentional, then I'd expect
    • The behaviour to be documented here
    • To respect finally blocks (omitted from my example to keep it short/focused)
    • To exit with a nonzero status (docs say "If the worker was terminated, the exitCode parameter will be 1")
@addaleax addaleax added confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support. labels Jun 9, 2019
@himself65
Copy link
Member

it also occurs on v12.3.1

@addaleax addaleax self-assigned this Jun 10, 2019
addaleax added a commit to addaleax/node that referenced this issue 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: nodejs#28144
@Trott Trott closed this as completed in d5cf51d Jun 20, 2019
targos pushed a commit that referenced this issue Jul 2, 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
PR-URL: #28153
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants