Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 39583f7

Browse filesBrowse 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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
1 parent 6330d43 commit 39583f7
Copy full SHA for 39583f7

File tree

Expand file treeCollapse file tree

3 files changed

+37
-5
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+37
-5
lines changed
Open diff view settings
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+14-4Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,10 @@ void Worker::Run() {
334334
Debug(this, "Created Environment for worker with id %llu", thread_id_.id);
335335
if (is_stopped()) return;
336336
{
337-
CreateEnvMessagePort(env_.get());
337+
if (!CreateEnvMessagePort(env_.get())) {
338+
return;
339+
}
340+
338341
Debug(this, "Created message port for worker %llu", thread_id_.id);
339342
if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty())
340343
return;
@@ -389,17 +392,24 @@ void Worker::Run() {
389392
Debug(this, "Worker %llu thread stops", thread_id_.id);
390393
}
391394

392-
void Worker::CreateEnvMessagePort(Environment* env) {
395+
bool Worker::CreateEnvMessagePort(Environment* env) {
393396
HandleScope handle_scope(isolate_);
394-
Mutex::ScopedLock lock(mutex_);
397+
std::unique_ptr<MessagePortData> data;
398+
{
399+
Mutex::ScopedLock lock(mutex_);
400+
data = std::move(child_port_data_);
401+
}
402+
395403
// Set up the message channel for receiving messages in the child.
396404
MessagePort* child_port = MessagePort::New(env,
397405
env->context(),
398-
std::move(child_port_data_));
406+
std::move(data));
399407
// MessagePort::New() may return nullptr if execution is terminated
400408
// within it.
401409
if (child_port != nullptr)
402410
env->set_message_port(child_port->object(isolate_));
411+
412+
return child_port;
403413
}
404414

405415
void Worker::JoinThread() {
Collapse file

‎src/node_worker.h‎

Copy file name to clipboardExpand all lines: src/node_worker.h
+1-1Lines changed: 1 addition & 1 deletion
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

Collapse file
+22Lines changed: 22 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.