From eee4378bf4464af9fe052ba3d5de35f373991efe Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 3 Feb 2020 00:06:35 -0500 Subject: [PATCH 1/4] src,worker: runtime error on loop creation failure Instead of hard asserting throw a runtime error, that is more consumable. Fixes: https://github.com/nodejs/node/issues/31614 --- doc/api/errors.md | 5 +++++ lib/internal/errors.js | 1 + lib/internal/worker.js | 13 +++++++++---- src/node_worker.cc | 40 ++++++++++++++++++++++++++++++---------- src/node_worker.h | 4 +++- 5 files changed, 48 insertions(+), 15 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index b186275807aee7..b25427941a5ccb 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2047,6 +2047,11 @@ meaning of the error depends on the specific function. The WASI instance has already started. + +### `ERR_WORKER_INIT_FAILED` + +The `Worker` initialization failed. + ### `ERR_WORKER_INVALID_EXEC_ARGV` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 392a297070d986..1e09db6c7ee38c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1358,6 +1358,7 @@ E('ERR_VM_MODULE_NOT_MODULE', 'Provided module is not an instance of Module', Error); E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); E('ERR_WASI_ALREADY_STARTED', 'WASI instance has already started', Error); +E('ERR_WORKER_INIT_FAILED', 'Worker initialization failure: %s', Error); E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') => `Initiated Worker with ${msg}: ${errors.join(', ')}`, Error); diff --git a/lib/internal/worker.js b/lib/internal/worker.js index de626af1bef962..7fc45695851f8c 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -25,6 +25,8 @@ const { ERR_WORKER_UNSUPPORTED_EXTENSION, ERR_WORKER_INVALID_EXEC_ARGV, ERR_INVALID_ARG_TYPE, + // eslint-disable-next-line no-unused-vars + ERR_WORKER_INIT_FAILED, } = errorCodes; const { validateString } = require('internal/validators'); const { getOptionValue } = require('internal/options'); @@ -136,7 +138,9 @@ class Worker extends EventEmitter { throw new ERR_WORKER_INVALID_EXEC_ARGV( this[kHandle].invalidNodeOptions, 'invalid NODE_OPTIONS env variable'); } - this[kHandle].onexit = (code, customErr) => this[kOnExit](code, customErr); + this[kHandle].onexit = (code, customErr, customErrReason) => { + this[kOnExit](code, customErr, customErrReason); + }; this[kPort] = this[kHandle].messagePort; this[kPort].on('message', (data) => this[kOnMessage](data)); this[kPort].start(); @@ -181,14 +185,15 @@ class Worker extends EventEmitter { this[kHandle].startThread(); } - [kOnExit](code, customErr) { + [kOnExit](code, customErr, customErrReason) { debug(`[${threadId}] hears end event for Worker ${this.threadId}`); drainMessagePort(this[kPublicPort]); drainMessagePort(this[kPort]); this[kDispose](); if (customErr) { - debug(`[${threadId}] failing with custom error ${customErr}`); - this.emit('error', new errorCodes[customErr]()); + debug(`[${threadId}] failing with custom error ${customErr} \ + and with reason {customErrReason}`); + this.emit('error', new errorCodes[customErr](customErrReason)); } this.emit('exit', code); this.removeAllListeners(); diff --git a/src/node_worker.cc b/src/node_worker.cc index 22c69ea6fb0690..781ed805408dd9 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -134,7 +134,17 @@ class WorkerThreadData { public: explicit WorkerThreadData(Worker* w) : w_(w) { - CHECK_EQ(uv_loop_init(&loop_), 0); + int ret = uv_loop_init(&loop_); + if (ret != 0) { + char err_buf[128]; + uv_err_name_r(ret, err_buf, sizeof(err_buf)); + std::string error_str = SPrintF("ERR_WORKER_INIT_FAILED: %s", err_buf); + w->custom_error_ = "ERR_WORKER_INIT_FAILED"; + w->custom_error_str_ = err_buf; + w->loop_init_failed_ = true; + w->stopped_ = true; + return; + } std::shared_ptr allocator = ArrayBufferAllocator::Create(); @@ -147,6 +157,8 @@ 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; return; } @@ -204,11 +216,14 @@ class WorkerThreadData { isolate->Dispose(); // Wait until the platform has cleaned up all relevant resources. - while (!platform_finished) + while (!platform_finished) { + CHECK(!w_->loop_init_failed_); uv_run(&loop_, UV_RUN_ONCE); + } + } + if (!w_->loop_init_failed_) { + CheckedUvLoopClose(&loop_); } - - CheckedUvLoopClose(&loop_); } private: @@ -223,6 +238,7 @@ size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit, size_t initial_heap_limit) { Worker* worker = static_cast(data); worker->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; + worker->custom_error_str_ = "JS heap Out of Memory"; worker->Exit(1); // Give the current GC some extra leeway to let it finish rather than // crash hard. We are not going to perform further allocations anyway. @@ -242,6 +258,7 @@ void Worker::Run() { WorkerThreadData data(this); if (isolate_ == nullptr) return; + CHECK(!data.w_->loop_init_failed_); Debug(this, "Starting worker with id %llu", thread_id_); { @@ -287,9 +304,8 @@ void Worker::Run() { TryCatch try_catch(isolate_); context = NewContext(isolate_); if (context.IsEmpty()) { - // TODO(addaleax): Inform the target about the actual underlying - // failure. custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; + custom_error_str_ = "Failed to create new Context"; return; } } @@ -417,10 +433,14 @@ void Worker::JoinThread() { Undefined(env()->isolate())).Check(); Local args[] = { - Integer::New(env()->isolate(), exit_code_), - custom_error_ != nullptr ? - OneByteString(env()->isolate(), custom_error_).As() : - Null(env()->isolate()).As(), + Integer::New(env()->isolate(), exit_code_), + !custom_error_.empty() + ? OneByteString(env()->isolate(), custom_error_.c_str()).As() + : Null(env()->isolate()).As(), + !custom_error_str_.empty() + ? OneByteString(env()->isolate(), custom_error_str_.c_str()) + .As() + : Null(env()->isolate()).As(), }; MakeCallback(env()->onexit_string(), arraysize(args), args); diff --git a/src/node_worker.h b/src/node_worker.h index 0c6fd35c0ab032..8d5de5033cc302 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -84,7 +84,9 @@ class Worker : public AsyncWrap { mutable Mutex mutex_; bool thread_joined_ = true; - const char* custom_error_ = nullptr; + std::string custom_error_; + std::string custom_error_str_; + bool loop_init_failed_ = false; int exit_code_ = 0; uint64_t thread_id_ = -1; uintptr_t stack_base_ = 0; From a214d9c37f9ed6f0681508fa7eac5b8dfcb41fed Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 17 Feb 2020 04:35:55 -0500 Subject: [PATCH 2/4] lib,test: parameterize worker resource tests Based on the new way of propagating worker initialization failures, modify the tests so as to get specialized error messages. --- lib/internal/errors.js | 2 +- test/parallel/test-worker-resource-limits.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 1e09db6c7ee38c..38b175a953c83d 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1363,7 +1363,7 @@ E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') => `Initiated Worker with ${msg}: ${errors.join(', ')}`, Error); E('ERR_WORKER_NOT_RUNNING', 'Worker instance not running', Error); -E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit', +E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit: %s', Error); E('ERR_WORKER_PATH', 'The worker script filename must be an absolute path or a relative ' + diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js index 2d4ebbc0ce6011..b92db5daeaad99 100644 --- a/test/parallel/test-worker-resource-limits.js +++ b/test/parallel/test-worker-resource-limits.js @@ -25,7 +25,7 @@ if (!process.env.HAS_STARTED_WORKER) { })); w.on('error', common.expectsError({ code: 'ERR_WORKER_OUT_OF_MEMORY', - message: 'Worker terminated due to reaching memory limit' + message: 'Worker terminated due to reaching memory limit: JS heap Out of Memory' })); return; } From 275f35d9a3f2be17ca9103b328811e1024862c43 Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 17 Feb 2020 06:23:31 -0500 Subject: [PATCH 3/4] fixup: fix linter errors --- lib/internal/errors.js | 4 ++-- test/parallel/test-worker-resource-limits.js | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 38b175a953c83d..9aa8957cf631e2 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1363,8 +1363,8 @@ E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') => `Initiated Worker with ${msg}: ${errors.join(', ')}`, Error); E('ERR_WORKER_NOT_RUNNING', 'Worker instance not running', Error); -E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit: %s', - Error); +E('ERR_WORKER_OUT_OF_MEMORY', + 'Worker terminated due to reaching memory limit: %s', Error); E('ERR_WORKER_PATH', 'The worker script filename must be an absolute path or a relative ' + 'path starting with \'./\' or \'../\'. Received "%s"', diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js index b92db5daeaad99..aa13d6c4660f56 100644 --- a/test/parallel/test-worker-resource-limits.js +++ b/test/parallel/test-worker-resource-limits.js @@ -25,7 +25,8 @@ if (!process.env.HAS_STARTED_WORKER) { })); w.on('error', common.expectsError({ code: 'ERR_WORKER_OUT_OF_MEMORY', - message: 'Worker terminated due to reaching memory limit: JS heap Out of Memory' + message: 'Worker terminated due to reaching memory limit:\ + JS heap Out of Memory' })); return; } From 90c45cb3c4e4e9bda04e95f8d0370493ca4b6b22 Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Tue, 18 Feb 2020 00:51:17 -0500 Subject: [PATCH 4/4] fixup: address review comments --- src/node_worker.cc | 7 +++---- src/node_worker.h | 2 +- test/parallel/test-worker-resource-limits.js | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 781ed805408dd9..b7532f8242a31d 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -138,7 +138,6 @@ class WorkerThreadData { if (ret != 0) { char err_buf[128]; uv_err_name_r(ret, err_buf, sizeof(err_buf)); - std::string error_str = SPrintF("ERR_WORKER_INIT_FAILED: %s", err_buf); w->custom_error_ = "ERR_WORKER_INIT_FAILED"; w->custom_error_str_ = err_buf; w->loop_init_failed_ = true; @@ -238,7 +237,7 @@ size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit, size_t initial_heap_limit) { Worker* worker = static_cast(data); worker->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; - worker->custom_error_str_ = "JS heap Out of Memory"; + worker->custom_error_str_ = "JS heap out of memory"; worker->Exit(1); // Give the current GC some extra leeway to let it finish rather than // crash hard. We are not going to perform further allocations anyway. @@ -434,8 +433,8 @@ void Worker::JoinThread() { Local args[] = { Integer::New(env()->isolate(), exit_code_), - !custom_error_.empty() - ? OneByteString(env()->isolate(), custom_error_.c_str()).As() + custom_error_ != nullptr + ? OneByteString(env()->isolate(), custom_error_).As() : Null(env()->isolate()).As(), !custom_error_str_.empty() ? OneByteString(env()->isolate(), custom_error_str_.c_str()) diff --git a/src/node_worker.h b/src/node_worker.h index 8d5de5033cc302..dbd286109948da 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -84,7 +84,7 @@ class Worker : public AsyncWrap { mutable Mutex mutex_; bool thread_joined_ = true; - std::string custom_error_; + const char* custom_error_ = nullptr; std::string custom_error_str_; bool loop_init_failed_ = false; int exit_code_ = 0; diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js index aa13d6c4660f56..9332a132694e78 100644 --- a/test/parallel/test-worker-resource-limits.js +++ b/test/parallel/test-worker-resource-limits.js @@ -25,8 +25,8 @@ if (!process.env.HAS_STARTED_WORKER) { })); w.on('error', common.expectsError({ code: 'ERR_WORKER_OUT_OF_MEMORY', - message: 'Worker terminated due to reaching memory limit:\ - JS heap Out of Memory' + message: 'Worker terminated due to reaching memory limit: ' + + 'JS heap out of memory' })); return; }