src,worker: runtime error on loop creation failure by HarshithaKP · Pull Request #31621 · nodejs/node · GitHub

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

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.

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?

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)

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

@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.

@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.`)
  })
}

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.

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

CI: https://ci.nodejs.org/job/node-test-pull-request/29229/

Landed in 2d3717a 🎉

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

Instead of hard asserting throw a runtime error, that is more consumable. Fixes: #31614 PR-URL: #31621 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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.

Instead of hard asserting throw a runtime error, that is more consumable. Fixes: #31614 PR-URL: #31621 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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>

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

legendecas reviewed Feb 3, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

addaleax reviewed Feb 3, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

bnoordhuis reviewed Feb 3, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

Comment thread src/node_worker.cc Outdated

HarshithaKP force-pushed the node_worker_assertion branch from 1542ff7 to 856372f Compare February 4, 2020 07:47

bnoordhuis reviewed Feb 4, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

Comment thread src/node_worker.cc Outdated

addaleax reviewed Feb 4, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

addaleax reviewed Feb 5, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

Comment thread src/node_worker.cc Outdated

gireeshpunathil mentioned this pull request Feb 5, 2020

assertion failure in src/node_worker.cc with large number of workers #31614

Closed

addaleax reviewed Feb 14, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

HarshithaKP added 2 commits February 17, 2020 04:50

src,worker: runtime error on loop creation failure

eee4378

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

lib,test: parameterize worker resource tests

a214d9c

Based on the new way of propagating worker initialization failures, modify the tests so as to get specialized error messages.

HarshithaKP force-pushed the node_worker_assertion branch from 9eb0ef4 to a214d9c Compare February 17, 2020 09:51

fixup: fix linter errors

275f35d

addaleax approved these changes Feb 18, 2020

View reviewed changes

Comment thread src/node_worker.cc Outdated

Comment thread src/node_worker.cc Outdated

Comment thread src/node_worker.h Outdated

fixup: address review comments

90c45cb

addaleax closed this Feb 19, 2020

addaleax pushed a commit that referenced this pull request Feb 19, 2020

worker: emit runtime error on loop creation failure

2d3717a

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 mentioned this pull request Feb 20, 2020

worker_threads: node[9926]: ../src/node_worker.cc:117:node::worker::WorkerThreadData::WorkerThreadData(node::worker::Worker*): Assertion `(uv_loop_init(&loop_)) == (0)' failed. #29755

Closed

HarshithaKP mentioned this pull request Feb 24, 2020

test: Worker initialization failure test case #31929

Closed

4 tasks

codebytere pushed a commit that referenced this pull request Feb 27, 2020

worker: emit runtime error on loop creation failure

f2389eb

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 mentioned this pull request Feb 29, 2020

v13.10.0 proposal #32027

Merged

codebytere pushed a commit that referenced this pull request Mar 15, 2020

worker: emit runtime error on loop creation failure

e01dbd4

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

worker: emit runtime error on loop creation failure

5af11df

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 mentioned this pull request Mar 17, 2020

v12.16.2 proposal #32313

Merged

gireeshpunathil mentioned this pull request Mar 17, 2020

assertion failure in ../src/node_worker.cc:647 with 4K worker threads #32319

Closed

codebytere pushed a commit that referenced this pull request Mar 30, 2020

worker: emit runtime error on loop creation failure

65729f9

Instead of hard asserting throw a runtime error, that is more consumable. Fixes: #31614 PR-URL: #31621 Reviewed-By: Anna Henningsen <anna@addaleax.net>

gireeshpunathil mentioned this pull request Aug 12, 2020

test: remove error message checking in test-worker-init-failure #34727

Merged

2 tasks

src,worker: runtime error on loop creation failure by HarshithaKP · Pull Request #31621 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src,worker: runtime error on loop creation failure#31621HarshithaKP wants to merge 4 commits intonodejs:masternodejs/node:masterfrom HarshithaKP:node_worker_assertionHarshithaKP/node:node_worker_assertionCopy head branch name to clipboard

Conversation

HarshithaKP commented Feb 3, 2020

Checklist

Uh oh!

Uh oh!

HarshithaKP commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HarshithaKP commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HarshithaKP commented Feb 5, 2020

Uh oh!

addaleax commented Feb 13, 2020

Uh oh!

HarshithaKP commented Feb 14, 2020

Uh oh!

HarshithaKP commented Feb 14, 2020

Uh oh!

Uh oh!

HarshithaKP commented Feb 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 19, 2020

Uh oh!

addaleax commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src,worker: runtime error on loop creation failure#31621
HarshithaKP wants to merge 4 commits into
nodejs:masterfrom
HarshithaKP:node_worker_assertion

HarshithaKP commented Feb 5, 2020 •
edited
Loading