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-botnodejs-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
MemberAuthor

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
Copy link
MemberAuthor

HarshithaKP commented Feb 5, 2020

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

bash-4.2$./nodetest/parallel/test-worker-resource-limitsMismatched<anonymous>functioncalls.Expectedexactly1,actual0.atmustCall(/home/harshitha/node/test/common/index.js:325:10)atObject.expectsError(/home/harshitha/node/test/common/index.js:531:10)atObject.<anonymous>(/home/harshitha/node/test/parallel/test-worker-resource-limits.js:26:24)atModule._compile(internal/modules/cjs/loader.js:1208:30)atObject.Module._extensions..js(internal/modules/cjs/loader.js:1228:10)atModule.load(internal/modules/cjs/loader.js:1057:32)atFunction.Module._load(internal/modules/cjs/loader.js:952:14)atFunction.executeUserEntryPoint[asrunMain](internal/modules/run_main.js:71:12)atinternal/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
MemberAuthor

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
MemberAuthor

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

bash-4.2$./node--expose-internalsWelcometoNode.jsv14.0.0-pre.Type".help"formoreinformation.>consterrorCodes=require('internal/errors').codesundefined>constobj=newerrorCodes['ERR_WORKER_INIT_FAILED']('foo')undefined>objError[ERR_WORKER_INIT_FAILED]: Workerinitialization failed: foo at repl:1:13atScript.runInThisContext(vm.js:120:20)atREPLServer.defaultEval(repl.js:436:29)atbound(domain.js:429:14)atREPLServer.runBound[aseval](domain.js:442:12)atREPLServer.onLine(repl.js:763:10)atREPLServer.emit(events.js:333:22)atREPLServer.EventEmitter.emit(domain.js:485:12)atREPLServer.Interface._onLine(readline.js:329:10)atREPLServer.Interface._line(readline.js:658:8){code: 'ERR_WORKER_INIT_FAILED'}

But when I run the test it is failing.

bash-4.2$./nodebar10000internal/worker.js:192this.emit('error',newerrorCodes[customErr]());^ TypeError: errorCodes[customErr]isnotaconstructoratWorker.[kOnExit](internal/worker.js:192:26)atWorker.<computed>.onexit(internal/worker.js:140:62)

Right now I am stuck here.

@HarshithaKP
Copy link
MemberAuthor

@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');varer=0varex=0for(leti=0;i<+process.argv[2];++i){constworker=newWorker('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
Copy link
MemberAuthor

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 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]>
@codebyterecodebytere 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]>
@codebyterecodebytere 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 freeto 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++.workerIssues 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

@HarshithaKP@addaleax@nodejs-github-bot@bnoordhuis@devsnek@legendecas