Skip to content

Commit f50b9c1

Browse files
santigimenorichardlau
authored andcommitted
worker: avoid potential deadlock on NearHeapLimit
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: #38208 PR-URL: #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]>
1 parent 70e094a commit f50b9c1

File tree

3 files changed

+37
-5
lines changed

3 files changed

+37
-5
lines changed

src/node_worker.cc

+14-4
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,10 @@ void Worker::Run() {
331331
Debug(this, "Created Environment for worker with id %llu", thread_id_.id);
332332
if (is_stopped()) return;
333333
{
334-
CreateEnvMessagePort(env_.get());
334+
if (!CreateEnvMessagePort(env_.get())) {
335+
return;
336+
}
337+
335338
Debug(this, "Created message port for worker %llu", thread_id_.id);
336339
if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty())
337340
return;
@@ -383,17 +386,24 @@ void Worker::Run() {
383386
Debug(this, "Worker %llu thread stops", thread_id_.id);
384387
}
385388

386-
void Worker::CreateEnvMessagePort(Environment* env) {
389+
bool Worker::CreateEnvMessagePort(Environment* env) {
387390
HandleScope handle_scope(isolate_);
388-
Mutex::ScopedLock lock(mutex_);
391+
std::unique_ptr<MessagePortData> data;
392+
{
393+
Mutex::ScopedLock lock(mutex_);
394+
data = std::move(child_port_data_);
395+
}
396+
389397
// Set up the message channel for receiving messages in the child.
390398
MessagePort* child_port = MessagePort::New(env,
391399
env->context(),
392-
std::move(child_port_data_));
400+
std::move(data));
393401
// MessagePort::New() may return nullptr if execution is terminated
394402
// within it.
395403
if (child_port != nullptr)
396404
env->set_message_port(child_port->object(isolate_));
405+
406+
return child_port;
397407
}
398408

399409
void Worker::JoinThread() {

src/node_worker.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class Worker : public AsyncWrap {
7070
static void LoopStartTime(const v8::FunctionCallbackInfo<v8::Value>& args);
7171

7272
private:
73-
void CreateEnvMessagePort(Environment* env);
73+
bool CreateEnvMessagePort(Environment* env);
7474
static size_t NearHeapLimit(void* data, size_t current_heap_limit,
7575
size_t initial_heap_limit);
7676

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const { Worker } = require('worker_threads');
5+
6+
// Do not use isMainThread so that this test itself can be run inside a Worker.
7+
if (!process.env.HAS_STARTED_WORKER) {
8+
process.env.HAS_STARTED_WORKER = 1;
9+
const opts = {
10+
resourceLimits: {
11+
maxYoungGenerationSizeMb: 0,
12+
maxOldGenerationSizeMb: 0
13+
}
14+
};
15+
16+
const worker = new Worker(__filename, opts);
17+
worker.on('error', common.mustCall((err) => {
18+
assert.strictEqual(err.code, 'ERR_WORKER_OUT_OF_MEMORY');
19+
}));
20+
} else {
21+
setInterval(() => {}, 1);
22+
}

0 commit comments

Comments
 (0)