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

workers: worker doesn't write all data to stdout stream with NODE_OPTIONS --require #31777

Closed
lundibundi opened this issue Feb 13, 2020 · 0 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@lundibundi
Copy link
Member

  • Version: master
  • Platform: Linux (at least)
  • Subsystem: workers

What steps will reproduce the bug?

// main.js
'use strict';

const { Worker } = require('worker_threads');
const assert = require('assert');

process.on('unhandledRejection', (err) => { throw err; });

async function collectStream(readable) {
  readable.setEncoding('utf8');
  let data = '';
  for await (const chunk of readable) {
    data += chunk;
  }
  return data;
}

for (let i = 0; i < 100; i++) {
  const w = new Worker('console.log("B");', {
    env: { NODE_OPTIONS: '--require ./printA.js' },
    eval: true,
    stdout: true
  });
  w.on('exit', () => {
    collectStream(w.stdout).then((data) => {
      assert.strictEqual(data, 'A\nB\n');
    });
  });
}
// printA.js
console.log('A');

How often does it reproduce? Is there a required condition?

Quite often (2/10 runs), under heavy load the frequency is higher.

What is the expected behavior?

Worker prints all data to the stdout stream.

What do you see instead?

Worker only prints part of the written data to the stdout stream.

Example of the test case above if we replace assert with just console.log(JSON.stringify(data)):

 ➔ dev/node/node ./out/Release/node test-worker-stdout-finish.js                                                                                                                                                                             ⇡ master :: ● :: ⬡
"A\nB\n"
"A\nB\n"
"A\nB\n"
"A\nB\n"
"A\nB\n"
"A\nB\n"
"A\nB\n"
"A\n"
"A\n"
"A\nB\n"

Additional information

Weirdly enough this only happens for NODE_OPTIONS --require cases, if the worker is requireing the file by itself or just has multiple console.log statements the bug is not present.
This also happens without the eval: true if we just run a file with console.log("B").

/cc @nodejs/workers

@lundibundi lundibundi added the worker Issues and PRs related to Worker support. label Feb 13, 2020
addaleax added a commit to addaleax/node that referenced this issue May 18, 2020
The refcount of the internal communication port is relevant for
stdio, but the `port.unref()` call effectively resets any `.ref()`
calls happening during stdio operations happening before it.

Therefore, do the `.unref()` call before loading preload modules,
which may cause stdio operations.

Fixes: nodejs#31777
codebytere pushed a commit that referenced this issue Jun 18, 2020
The refcount of the internal communication port is relevant for
stdio, but the `port.unref()` call effectively resets any `.ref()`
calls happening during stdio operations happening before it.

Therefore, do the `.unref()` call before loading preload modules,
which may cause stdio operations.

Fixes: #31777

PR-URL: #33455
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
codebytere pushed a commit that referenced this issue Jul 8, 2020
The refcount of the internal communication port is relevant for
stdio, but the `port.unref()` call effectively resets any `.ref()`
calls happening during stdio operations happening before it.

Therefore, do the `.unref()` call before loading preload modules,
which may cause stdio operations.

Fixes: #31777

PR-URL: #33455
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant