Skip to content

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

@HarshithaKP HarshithaKP force-pushed the node_worker_assertion branch from 1542ff7 to 856372f Compare February 4, 2020 07:47
@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?

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

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 HarshithaKP force-pushed the node_worker_assertion branch from 9eb0ef4 to a214d9c Compare February 17, 2020 09:51
@HarshithaKP
Copy link
Member Author

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

@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 <[email protected]>
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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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