Skip to content

Commit

Permalink
worker: fix heap snapshot crash on exit
Browse files Browse the repository at this point in the history
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
legendecas authored and juanarbol committed May 31, 2022
1 parent 236d139 commit 835f0a5
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 23 deletions.
16 changes: 6 additions & 10 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,18 @@ Worker::Worker(Environment* env,
thread_id_.id);

// Set up everything that needs to be set up in the parent environment.
parent_port_ = MessagePort::New(env, env->context());
if (parent_port_ == nullptr) {
MessagePort* parent_port = MessagePort::New(env, env->context());
if (parent_port == nullptr) {
// This can happen e.g. because execution is terminating.
return;
}

child_port_data_ = std::make_unique<MessagePortData>(nullptr);
MessagePort::Entangle(parent_port_, child_port_data_.get());
MessagePort::Entangle(parent_port, child_port_data_.get());

object()->Set(env->context(),
env->message_port_string(),
parent_port_->object()).Check();
object()
->Set(env->context(), env->message_port_string(), parent_port->object())
.Check();

object()->Set(env->context(),
env->thread_id_string(),
Expand Down Expand Up @@ -724,10 +724,6 @@ void Worker::Exit(int code, const char* error_code, const char* error_message) {
}
}

void Worker::MemoryInfo(MemoryTracker* tracker) const {
tracker->TrackField("parent_port", parent_port_);
}

bool Worker::IsNotIndicativeOfMemoryLeakAtExit() const {
// Worker objects always stay alive as long as the child thread, regardless
// of whether they are being referenced in the parent thread.
Expand Down
6 changes: 1 addition & 5 deletions src/node_worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class Worker : public AsyncWrap {
template <typename Fn>
inline bool RequestInterrupt(Fn&& cb);

void MemoryInfo(MemoryTracker* tracker) const override;
SET_NO_MEMORY_INFO()
SET_MEMORY_INFO_NAME(Worker)
SET_SELF_SIZE(Worker)
bool IsNotIndicativeOfMemoryLeakAtExit() const override;
Expand Down Expand Up @@ -107,10 +107,6 @@ class Worker : public AsyncWrap {
std::unique_ptr<MessagePortData> child_port_data_;
std::shared_ptr<KVStore> env_vars_;

// This is always kept alive because the JS object associated with the Worker
// instance refers to it via its [kPort] property.
MessagePort* parent_port_ = nullptr;

// A raw flag that is used by creator and worker threads to
// sync up on pre-mature termination of worker - while in the
// warmup phase. Once the worker is fully warmed up, use the
Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-worker-exit-heapsnapshot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { getHeapSnapshot } = require('v8');
const { isMainThread, Worker } = require('worker_threads');

// Checks taking heap snapshot at the exit event listener of Worker doesn't
// crash the process.
// Regression for https://github.com/nodejs/node/issues/43122.
if (isMainThread) {
const worker = new Worker(__filename);

worker.once('exit', common.mustCall((code) => {
assert.strictEqual(code, 0);
getHeapSnapshot().pipe(process.stdout);
}));
}
8 changes: 0 additions & 8 deletions test/pummel/test-heapdump-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,6 @@ const { Worker } = require('worker_threads');

validateSnapshotNodes('Node / Worker', []);
const worker = new Worker('setInterval(() => {}, 100);', { eval: true });
validateSnapshotNodes('Node / Worker', [
{
children: [
{ node_name: 'Node / MessagePort', edge_name: 'parent_port' },
{ node_name: 'Worker', edge_name: 'wrapped' },
]
},
]);
validateSnapshotNodes('Node / MessagePort', [
{
children: [
Expand Down

0 comments on commit 835f0a5

Please sign in to comment.