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 pthread creation failure #32344

Closed
wants to merge 4 commits into from

Conversation

HarshithaKP
Copy link
Member

@HarshithaKP HarshithaKP commented Mar 18, 2020

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

@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 Mar 18, 2020
@HarshithaKP
Copy link
Member Author

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

jasnell commented Mar 18, 2020

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
Copy link
Member Author

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?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error, so the caller can catch?

Yes, I’d suggest doing that.

// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 661 to 663
w->object()->Set(w->env()->context(),
w->env()->message_port_string(),
Undefined(w->env()->isolate())).Check();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().)

Comment on lines 664 to 673
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>(),
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@addaleax and @bnoordhuis, PTAL.

HandleScope handle_scope(isolate);
THROW_ERR_WORKER_INIT_FAILED(isolate, err_buf);
}
w->MakeWeak();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
w->MakeWeak();
w->MakeWeak();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis, thanks. Fixed it.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert the file permissions back to 0644

@HarshithaKP
Copy link
Member Author

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

@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in b6459ec

@addaleax addaleax closed this Mar 30, 2020
addaleax pushed a commit that referenced this pull request Mar 30, 2020
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>
addaleax added a commit to addaleax/node that referenced this pull request Mar 30, 2020
Instead of setting and then in the case of error un-setting properties,
only set them when no error occurs.

Refs: nodejs#32344
@addaleax addaleax mentioned this pull request Mar 30, 2020
2 tasks
addaleax pushed a commit that referenced this pull request Mar 30, 2020
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>
addaleax added a commit that referenced this pull request Apr 2, 2020
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>
addaleax added a commit that referenced this pull request Apr 2, 2020
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>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
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>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
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>
targos pushed a commit that referenced this pull request Apr 12, 2020
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>
targos pushed a commit that referenced this pull request Apr 12, 2020
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>
@targos 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 pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
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:647 with 4K worker threads
7 participants