Skip to content

Commit

Permalink
worker: unify custom error creation
Browse files Browse the repository at this point in the history
Mostly, this introduces a pattern that makes sure that if a custom
error is reported, `stopped_` will be set to `true` correctly in
every cast, which was previously missing for the
`NewContext().IsEmpty()` case (which led to a hard crash from the
`Worker` destructor).

This also leaves TODO comments for a few cases in which
`ERR_WORKER_OUT_OF_MEMORY` was not used in accordance with the
documentation for that error code (or according to its intention).
Fixing that is semver-major.

PR-URL: #33084
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and targos committed May 4, 2020
1 parent 3c2f608 commit c6d632a
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 15 deletions.
30 changes: 17 additions & 13 deletions src/node_worker.cc
Expand Up @@ -131,9 +131,7 @@ class WorkerThreadData {
if (ret != 0) {
char err_buf[128];
uv_err_name_r(ret, err_buf, sizeof(err_buf));
w->custom_error_ = "ERR_WORKER_INIT_FAILED";
w->custom_error_str_ = err_buf;
w->stopped_ = true;
w->Exit(1, "ERR_WORKER_INIT_FAILED", err_buf);
return;
}
loop_init_failed_ = false;
Expand All @@ -148,9 +146,9 @@ class WorkerThreadData {

Isolate* isolate = Isolate::Allocate();
if (isolate == nullptr) {
w->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY";
w->custom_error_str_ = "Failed to create new Isolate";
w->stopped_ = true;
// TODO(addaleax): This should be ERR_WORKER_INIT_FAILED,
// ERR_WORKER_OUT_OF_MEMORY is for reaching the per-Worker heap limit.
w->Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "Failed to create new Isolate");
return;
}

Expand Down Expand Up @@ -233,9 +231,7 @@ class WorkerThreadData {
size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit,
size_t initial_heap_limit) {
Worker* worker = static_cast<Worker*>(data);
worker->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY";
worker->custom_error_str_ = "JS heap out of memory";
worker->Exit(1);
worker->Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "JS heap out of memory");
// Give the current GC some extra leeway to let it finish rather than
// crash hard. We are not going to perform further allocations anyway.
constexpr size_t kExtraHeapAllowance = 16 * 1024 * 1024;
Expand Down Expand Up @@ -292,8 +288,9 @@ void Worker::Run() {
TryCatch try_catch(isolate_);
context = NewContext(isolate_);
if (context.IsEmpty()) {
custom_error_ = "ERR_WORKER_OUT_OF_MEMORY";
custom_error_str_ = "Failed to create new Context";
// TODO(addaleax): This should be ERR_WORKER_INIT_FAILED,
// ERR_WORKER_OUT_OF_MEMORY is for reaching the per-Worker heap limit.
Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "Failed to create new Context");
return;
}
}
Expand Down Expand Up @@ -682,9 +679,16 @@ Local<Float64Array> Worker::GetResourceLimits(Isolate* isolate) const {
return Float64Array::New(ab, 0, kTotalResourceLimitCount);
}

void Worker::Exit(int code) {
void Worker::Exit(int code, const char* error_code, const char* error_message) {
Mutex::ScopedLock lock(mutex_);
Debug(this, "Worker %llu called Exit(%d)", thread_id_.id, code);
Debug(this, "Worker %llu called Exit(%d, %s, %s)",
thread_id_.id, code, error_code, error_message);

if (error_code != nullptr) {
custom_error_ = error_code;
custom_error_str_ = error_message;
}

if (env_ != nullptr) {
exit_code_ = code;
Stop(env_);
Expand Down
7 changes: 5 additions & 2 deletions src/node_worker.h
Expand Up @@ -34,8 +34,11 @@ class Worker : public AsyncWrap {
void Run();

// Forcibly exit the thread with a specified exit code. This may be called
// from any thread.
void Exit(int code);
// from any thread. `error_code` and `error_message` can be used to create
// a custom `'error'` event before emitting `'exit'`.
void Exit(int code,
const char* error_code = nullptr,
const char* error_message = nullptr);

// Wait for the worker thread to stop (in a blocking manner).
void JoinThread();
Expand Down

0 comments on commit c6d632a

Please sign in to comment.