From 598356820459b1c3b32f7647c418278a6430619e Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Sun, 25 Apr 2021 18:42:47 +0200 Subject: [PATCH] worker: avoid potential deadlock on NearHeapLimit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It can happen that the `NearHeapLimit` callback is called while calling the `oninit()` function on `MessagePort` construction causing a deadlock when the `Worker::Exit()` method is called, as the `mutex_` was already held on the `CreateEnvMessagePort()` method. To fix it, just use the `mutex_` to protect the `child_port_data_` variable and avoid holding it when creating the `MessagePort`. Also, return early from `Worker::Run()` if the worker message port could not be created. Fixes: https://github.com/nodejs/node/issues/38208 PR-URL: https://github.com/nodejs/node/pull/38403 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Joyee Cheung Reviewed-By: Rich Trott Reviewed-By: Juan José Arboleda --- src/node_worker.cc | 18 +++++++++++---- src/node_worker.h | 2 +- .../test-worker-nearheaplimit-deadlock.js | 22 +++++++++++++++++++ 3 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-worker-nearheaplimit-deadlock.js diff --git a/src/node_worker.cc b/src/node_worker.cc index 3195ddbaf01b87..dcc76ad88f04e2 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -328,7 +328,10 @@ void Worker::Run() { Debug(this, "Created Environment for worker with id %llu", thread_id_.id); if (is_stopped()) return; { - CreateEnvMessagePort(env_.get()); + if (!CreateEnvMessagePort(env_.get())) { + return; + } + Debug(this, "Created message port for worker %llu", thread_id_.id); if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty()) return; @@ -352,17 +355,24 @@ void Worker::Run() { Debug(this, "Worker %llu thread stops", thread_id_.id); } -void Worker::CreateEnvMessagePort(Environment* env) { +bool Worker::CreateEnvMessagePort(Environment* env) { HandleScope handle_scope(isolate_); - Mutex::ScopedLock lock(mutex_); + std::unique_ptr data; + { + Mutex::ScopedLock lock(mutex_); + data = std::move(child_port_data_); + } + // Set up the message channel for receiving messages in the child. MessagePort* child_port = MessagePort::New(env, env->context(), - std::move(child_port_data_)); + std::move(data)); // MessagePort::New() may return nullptr if execution is terminated // within it. if (child_port != nullptr) env->set_message_port(child_port->object(isolate_)); + + return child_port; } void Worker::JoinThread() { diff --git a/src/node_worker.h b/src/node_worker.h index 2c65f0e1a83bbe..077d2b8390e6f8 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -70,7 +70,7 @@ class Worker : public AsyncWrap { static void LoopStartTime(const v8::FunctionCallbackInfo& args); private: - void CreateEnvMessagePort(Environment* env); + bool CreateEnvMessagePort(Environment* env); static size_t NearHeapLimit(void* data, size_t current_heap_limit, size_t initial_heap_limit); diff --git a/test/parallel/test-worker-nearheaplimit-deadlock.js b/test/parallel/test-worker-nearheaplimit-deadlock.js new file mode 100644 index 00000000000000..1f38bc074da048 --- /dev/null +++ b/test/parallel/test-worker-nearheaplimit-deadlock.js @@ -0,0 +1,22 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Do not use isMainThread so that this test itself can be run inside a Worker. +if (!process.env.HAS_STARTED_WORKER) { + process.env.HAS_STARTED_WORKER = 1; + const opts = { + resourceLimits: { + maxYoungGenerationSizeMb: 0, + maxOldGenerationSizeMb: 0 + } + }; + + const worker = new Worker(__filename, opts); + worker.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_WORKER_OUT_OF_MEMORY'); + })); +} else { + setInterval(() => {}, 1); +}