Skip to content

Commit

Permalink
worker: avoid potential deadlock on NearHeapLimit
Browse files Browse the repository at this point in the history
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: nodejs#38208

PR-URL: nodejs#38403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
  • Loading branch information
santigimeno authored and juanarbol committed Sep 11, 2021
1 parent ec555b0 commit 540f9d9
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 5 deletions.
18 changes: 14 additions & 4 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<MessagePortData> 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() {
Expand Down
2 changes: 1 addition & 1 deletion src/node_worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class Worker : public AsyncWrap {
static void LoopStartTime(const v8::FunctionCallbackInfo<v8::Value>& 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);

Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-worker-nearheaplimit-deadlock.js
Original file line number Diff line number Diff line change
@@ -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);
}

0 comments on commit 540f9d9

Please sign in to comment.