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

src,worker: runtime error on loop creation failure #31621

Closed
wants to merge 4 commits into from

Conversation

HarshithaKP
Copy link
Member

Instead of hard asserting throw a runtime error,
that is more consumable.
Fixes: #31614

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Feb 3, 2020
src/node_worker.cc Outdated Show resolved Hide resolved
@HarshithaKP
Copy link
Member Author

When I run the test case mentioned in the issue with this change, getting the following error.

node: ../deps/uv/src/unix/core.c:556: uv__close_nocheckstdio: Assertion `fd > -1' failed.

src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
@HarshithaKP
Copy link
Member Author

HarshithaKP commented Feb 5, 2020

With this change in place, test parallel/test-worker-resource-limits is failing with this stack:

bash-4.2$ ./node test/parallel/test-worker-resource-limits
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at mustCall (/home/harshitha/node/test/common/index.js:325:10)
    at Object.expectsError (/home/harshitha/node/test/common/index.js:531:10)
    at Object.<anonymous> (/home/harshitha/node/test/parallel/test-worker-resource-limits.js:26:24)
    at Module._compile (internal/modules/cjs/loader.js:1208:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1228:10)
    at Module.load (internal/modules/cjs/loader.js:1057:32)
    at Function.Module._load (internal/modules/cjs/loader.js:952:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

I believe it is not because of the mismatch in the error code, but because the error callback is not invoked. I am able to recreate with the test in the referenced issue: missing error callback. My original intent with this PR was to covert C++ assertion into an error callback, which is missing! any guidance here?

src/node_worker.cc Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
@HarshithaKP
Copy link
Member Author

Getting this currently, I will debug further.

bash-4.2$ ./node bar.js 10000
internal/worker.js:191
      this.emit('error', new errorCodes[customErr]());
                         ^
TypeError: errorCodes[customErr] is not a constructor
    at Worker.[kOnExit] (internal/worker.js:191:26)
    at Worker.<computed>.onexit (internal/worker.js:139:62)

@addaleax
Copy link
Member

@HarshithaKP Hi! Let me/us know if you need anything to make this PR work :)

@HarshithaKP
Copy link
Member Author

@addaleax, Thanks.
When I check the constructor with a string value, it works as follows.

bash-4.2$ ./node --expose-internals
Welcome to Node.js v14.0.0-pre.
Type ".help" for more information.
> const errorCodes = require('internal/errors').codes
undefined
> const obj = new errorCodes['ERR_WORKER_INIT_FAILED']('foo')
undefined
> obj
Error [ERR_WORKER_INIT_FAILED]: Worker initialization failed: foo
    at repl:1:13
    at Script.runInThisContext (vm.js:120:20)
    at REPLServer.defaultEval (repl.js:436:29)
    at bound (domain.js:429:14)
    at REPLServer.runBound [as eval] (domain.js:442:12)
    at REPLServer.onLine (repl.js:763:10)
    at REPLServer.emit (events.js:333:22)
    at REPLServer.EventEmitter.emit (domain.js:485:12)
    at REPLServer.Interface._onLine (readline.js:329:10)
    at REPLServer.Interface._line (readline.js:658:8) {
  code: 'ERR_WORKER_INIT_FAILED'
}

But when I run the test it is failing.

bash-4.2$ ./node bar 10000
internal/worker.js:192
      this.emit('error', new errorCodes[customErr]());
                         ^

TypeError: errorCodes[customErr] is not a constructor
    at Worker.[kOnExit] (internal/worker.js:192:26)
    at Worker.<computed>.onexit (internal/worker.js:140:62)

Right now I am stuck here.

@HarshithaKP
Copy link
Member Author

@addaleax, thanks again for following up and offer to help!
In the above test case, bar contains creation of a number of workers that will fail eventually due to lack of resource. I would expect an error stack similar to above, but passed to the worker’s error callback, not thrown uncaught the way it did.

$ cat bar.js

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

var er = 0
var ex = 0
for (let i = 0; i < +process.argv[2]; ++i) {
  const worker = new Worker(
    'require(\'worker_threads\').parentPort.postMessage(2 + 2)',
    { eval: true });
  worker.on('error', (a, b, c) => {
    console.log(`${er++} err'd!`)
    console.log(a)
    console.log(b)
    console.log(c)
  })
  worker.on('exit', () => {
    console.log(`${ex++} exited.`)
  })
}

src/node_worker.cc Outdated Show resolved Hide resolved
Instead of hard asserting throw a runtime error,
that is more consumable.
Fixes: nodejs#31614
Based on the new way of propagating worker initialization
failures, modify the tests so as to get specialized
error messages.
@HarshithaKP
Copy link
Member Author

The changes were resulting in node_worker_assertion.js test failure. Fixed it in one more commit.

src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.h Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in 2d3717a 🎉

@HarshithaKP Thanks for persisting and bringing this over the finish line!

@addaleax addaleax closed this Feb 19, 2020
addaleax pushed a commit that referenced this pull request Feb 19, 2020
Instead of hard asserting throw a runtime error,
that is more consumable.

Fixes: #31614

PR-URL: #31621
Reviewed-By: Anna Henningsen <anna@addaleax.net>
HarshithaKP added a commit to HarshithaKP/node that referenced this pull request Feb 24, 2020
Cover the scenario fixed through
nodejs#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.
codebytere pushed a commit that referenced this pull request Feb 27, 2020
Instead of hard asserting throw a runtime error,
that is more consumable.

Fixes: #31614

PR-URL: #31621
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Feb 29, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Instead of hard asserting throw a runtime error,
that is more consumable.

Fixes: #31614

PR-URL: #31621
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Instead of hard asserting throw a runtime error,
that is more consumable.

Fixes: #31614

PR-URL: #31621
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Instead of hard asserting throw a runtime error,
that is more consumable.

Fixes: #31614

PR-URL: #31621
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Apr 5, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 12, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 22, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assertion failure in src/node_worker.cc with large number of workers
6 participants