src,worker: runtime error on pthread creation failure #32344

HarshithaKP · 2020-03-18T14:47:09Z

with large number of worker threads, uv_pthread_create_ex was failing,
that leads to assertion failure. Convert it into a runtime error.

Fixes: #32319

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

HarshithaKP · 2020-03-18T14:57:59Z

events.js:292
      throw er; // Unhandled 'error' event
      ^
Error [ERR_WORKER_INIT_FAILED]: Worker initialization failure: EAGAIN
    at Worker.[kOnExit] (internal/worker.js:216:26)
    at Worker.<computed>.onexit (internal/worker.js:162:20)
    at Worker.startThread (<anonymous>)
    at new Worker (internal/worker.js:205:19)
    at Object.<anonymous> (/foo/wm/foo.js:6:19)
    at Module._compile (internal/modules/cjs/loader.js:1202:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1222:10)
    at Module.load (internal/modules/cjs/loader.js:1051:32)
    at Function.Module._load (internal/modules/cjs/loader.js:947:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
Emitted 'error' event on Worker instance at:
    at Worker.[kOnExit] (internal/worker.js:216:12)
    at Worker.<computed>.onexit (internal/worker.js:162:20)
    [... lines matching original stack trace ...]
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
  code: 'ERR_WORKER_INIT_FAILED'
}

Instead of assertion failure, now we get a runtime error like this.

jasnell · 2020-03-18T23:30:51Z

Change looks good. Ideally this would come with a test but given the nature of the failure that's going to be difficult I'd imagine.

HarshithaKP · 2020-03-20T05:16:26Z

when I was doing testing with this, I found this out: the error was not actually caught by the error handler, instead thrown as unahndled exception.
debugging further, I found out that the location of failure (uv_pthread_create) is too early in the worker construction (compared to the previous case where it was failing in the worker thread itself), and when the emit happens the callback was not yet registered.
not sure what to do here:

delay emitting till the next tick?
throw error, so the caller can catch?

addaleax

throw error, so the caller can catch?

Yes, I’d suggest doing that.

addaleax · 2020-03-20T09:27:14Z

src/node_worker.cc

+      // Reset the parent port as we're closing it now anyway.
+      w->object()->Set(w->env()->context(),
+                    w->env()->message_port_string(),
+                    Undefined(w->env()->isolate())).Check();


Please return here if the result of Set() is empty instead of using .Check().

bnoordhuis · 2020-03-20T09:51:20Z

src/node_worker.cc

+      w->object()->Set(w->env()->context(),
+                    w->env()->message_port_string(),
+                    Undefined(w->env()->isolate())).Check();


Suggested change

w->object()->Set(w->env()->context(),

w->env()->message_port_string(),

Undefined(w->env()->isolate())).Check();

w->object()->Set(w->env()->context(),

w->env()->message_port_string(),

Undefined(w->env()->isolate())).Check();

(arguments should line up - but like Anna said, don't use Check().)

bnoordhuis · 2020-03-20T10:01:29Z

src/node_worker.cc

+      Local<Value> args[] = {
+          Integer::New(w->env()->isolate(), w->exit_code_),
+          w->custom_error_ != nullptr
+              ? OneByteString(w->env()->isolate(), w->custom_error_).As<Value>()
+              : Null(w->env()->isolate()).As<Value>(),
+          !w->custom_error_str_.empty()
+              ? OneByteString(w->env()->isolate(), w->custom_error_str_.c_str())
+                    .As<Value>()
+              : Null(w->env()->isolate()).As<Value>(),
+      };


I find this ternary-heave style very hard to read. Can I suggest this?

Isolate* isolate = w->env()->isolate(); HandleScope handle_scope(isolate); // ... Local<Value> exit_code_v = Integer::New(isolate, w->exit_code_); Local<Value> null_v = Null(isolate).As<Value>(); Local<Value> argv[3] = {exit_code_v, null_v, null_v}; if (w->custom_error_ != nullptr) args[1] = OneByteString(isolate, w->custom_error_); if (!w->custom_error_str_.empty()) args[2] = OneByteString(isolate, w->custom_error_str_.c_str());

Or alternatively:

Local<Value> exit_code_v = Integer::New(isolate, w->exit_code_); Local<Value> null_v = Null(isolate).As<Value>(); Local<Value> custom_error_v = null_v; Local<Value> custom_error_str_v = null_v; if (w->custom_error_ != nullptr) custom_error_v = OneByteString(isolate, w->custom_error_); if (!w->custom_error_str_.empty()) custom_error_str_v = OneByteString(isolate, w->custom_error_str_.c_str()); Local<Value> argv[] = {exit_code_v, custom_error_v, custom_error_str_v};

I've left out the .As<Value>() casts because I don't think they're necessary here.

Fwiw, we don’t need the ternaries here anyway because we already know that custom_error_ and custom_error_str_ are set.

But either way, that should go away if this is converted to throwing an exception.

with large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. Refs: nodejs#32319

HarshithaKP · 2020-03-21T06:57:24Z

@addaleax and @bnoordhuis, PTAL.

bnoordhuis · 2020-03-21T13:23:38Z

src/node_worker.cc

+      HandleScope handle_scope(isolate);
+      THROW_ERR_WORKER_INIT_FAILED(isolate, err_buf);
+    }
+  w->MakeWeak();


Suggested change

w->MakeWeak();

w->MakeWeak();

@bnoordhuis, thanks. Fixed it.

devsnek

please revert the file permissions back to 0644

HarshithaKP · 2020-03-23T10:50:21Z

@devsnek, thanks. reverted the file permissions to 644. PTAL.

addressed

nodejs-github-bot · 2020-03-29T20:10:41Z

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

nodejs-github-bot · 2020-03-29T22:40:50Z

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

addaleax · 2020-03-30T08:27:50Z

Landed in b6459ec

With large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. PR-URL: #32344 Fixes: #32319 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: nodejs#32344

With large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. PR-URL: #32344 Fixes: #32319 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: #32344 PR-URL: #32562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There’s no reason for this to not be stored alongside the loop data structure itself. PR-URL: #32562 Refs: #32344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: #32344 PR-URL: #32562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There’s no reason for this to not be stored alongside the loop data structure itself. PR-URL: #32562 Refs: #32344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: #32344 PR-URL: #32562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There’s no reason for this to not be stored alongside the loop data structure itself. PR-URL: #32562 Refs: #32344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

With large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. PR-URL: nodejs#32344 Fixes: nodejs#32319 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: nodejs#32344 PR-URL: nodejs#32562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There’s no reason for this to not be stored alongside the loop data structure itself. PR-URL: nodejs#32562 Refs: nodejs#32344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

With large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. PR-URL: #32344 Fixes: #32319 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: #32344 PR-URL: #32562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There’s no reason for this to not be stored alongside the loop data structure itself. PR-URL: #32562 Refs: #32344 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 Mar 18, 2020

addaleax reviewed Mar 20, 2020

View reviewed changes

bnoordhuis reviewed Mar 20, 2020

View reviewed changes

HarshithaKP added 2 commits March 21, 2020 02:48

src,worker: runtime error on pthread creation

9fcd394

with large number of worker threads pthread fails with hard assertion. Instead of hard assertion throw a runtime error. Refs: nodejs#32319

fixup: convert emit to throw

c064d2d

HarshithaKP force-pushed the pthread_create_failure branch from 831cffc to c064d2d Compare March 21, 2020 06:53

bnoordhuis approved these changes Mar 21, 2020

View reviewed changes

fixup: address review comment

2f5a48d

devsnek previously requested changes Mar 23, 2020

View reviewed changes

fixup: address review comment

9a5b658

addaleax approved these changes Mar 29, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020

jasnell approved these changes Mar 29, 2020

View reviewed changes

addaleax closed this Mar 30, 2020

addaleax added a commit to addaleax/node that referenced this pull request Mar 30, 2020

src: clean up worker thread creation code

09d3469

Instead of setting and then in the case of error un-setting properties, only set them when no error occurs. Refs: nodejs#32344

addaleax mentioned this pull request Mar 30, 2020

src: minor worker code cleanups #32562

Closed

2 tasks

targos mentioned this pull request Apr 13, 2020

v13.13.0 release proposal #32813

Merged

targos added backport-blocked-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 22, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,worker: runtime error on pthread creation failure #32344

src,worker: runtime error on pthread creation failure #32344

HarshithaKP commented Mar 18, 2020 •

edited

Loading

HarshithaKP commented Mar 18, 2020

jasnell commented Mar 18, 2020

HarshithaKP commented Mar 20, 2020

addaleax left a comment

addaleax Mar 20, 2020

bnoordhuis Mar 20, 2020

bnoordhuis Mar 20, 2020

addaleax Mar 20, 2020

HarshithaKP commented Mar 21, 2020

bnoordhuis Mar 21, 2020

HarshithaKP Mar 23, 2020

devsnek left a comment

HarshithaKP commented Mar 23, 2020

nodejs-github-bot commented Mar 29, 2020

nodejs-github-bot commented Mar 29, 2020

addaleax commented Mar 30, 2020

src,worker: runtime error on pthread creation failure #32344

src,worker: runtime error on pthread creation failure #32344

Conversation

HarshithaKP commented Mar 18, 2020 • edited Loading

Checklist

HarshithaKP commented Mar 18, 2020

jasnell commented Mar 18, 2020

HarshithaKP commented Mar 20, 2020

addaleax left a comment

Choose a reason for hiding this comment

addaleax Mar 20, 2020

Choose a reason for hiding this comment

bnoordhuis Mar 20, 2020

Choose a reason for hiding this comment

bnoordhuis Mar 20, 2020

Choose a reason for hiding this comment

addaleax Mar 20, 2020

Choose a reason for hiding this comment

HarshithaKP commented Mar 21, 2020

bnoordhuis Mar 21, 2020

Choose a reason for hiding this comment

HarshithaKP Mar 23, 2020

Choose a reason for hiding this comment

devsnek left a comment

Choose a reason for hiding this comment

HarshithaKP commented Mar 23, 2020

nodejs-github-bot commented Mar 29, 2020

nodejs-github-bot commented Mar 29, 2020

addaleax commented Mar 30, 2020

HarshithaKP commented Mar 18, 2020 •

edited

Loading