Skip to content

Commit

Permalink
src: restrict unloading addons to Worker threads
Browse files Browse the repository at this point in the history
Unloading native addons from the main thread was an (presumably
unintended) significant breaking change, because addons may
rely on their memory being available after an `Environment` exits.

This patch only restricts this to Worker threads, at least for the
time being, and thus matches the behaviour from
nodejs#23319.

Refs: nodejs#24861
Refs: nodejs#23319
  • Loading branch information
addaleax committed Jan 22, 2019
1 parent 38ab1e9 commit 0934abd
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 6 deletions.
8 changes: 5 additions & 3 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,11 @@ Environment::~Environment() {
TRACE_EVENT_NESTABLE_ASYNC_END0(
TRACING_CATEGORY_NODE1(environment), "Environment", this);

// Dereference all addons that were loaded into this environment.
for (binding::DLib& addon : loaded_addons_) {
addon.Close();
if (!is_main_thread()) {
// Dereference all addons that were loaded into this environment.
for (binding::DLib& addon : loaded_addons_) {
addon.Close();
}
}
}

Expand Down
11 changes: 11 additions & 0 deletions test/addons/worker-addon/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <v8.h>
#include <uv.h>

using v8::Context;
using v8::HandleScope;
Expand Down Expand Up @@ -41,6 +42,16 @@ void Initialize(Local<Object> exports,
const_cast<void*>(static_cast<const void*>("cleanup")));
node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);

if (getenv("addExtraItemToEventLoop") != nullptr) {
// Add an item to the event loop that we do not clean up in order to make
// sure that for the main thread, this addon's memory persists even after
// the Environment instance has been destroyed.
static uv_async_t extra_async;
uv_loop_t* loop = node::GetCurrentEventLoop(context->GetIsolate());
uv_async_init(loop, &extra_async, [](uv_async_t*) {});
uv_unref(reinterpret_cast<uv_handle_t*>(&extra_async));
}
}

NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
13 changes: 10 additions & 3 deletions test/addons/worker-addon/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,19 @@ const path = require('path');
const { Worker } = require('worker_threads');
const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);

if (process.argv[2] === 'child') {
if (process.argv[2] === 'worker') {
new Worker(`require(${JSON.stringify(binding)});`, { eval: true });
} else {
return;
} else if (process.argv[2] === 'main-thread') {
process.env.addExtraItemToEventLoop = 'yes';
require(binding);
return;
}

for (const test of ['worker', 'main-thread']) {
const proc = child_process.spawnSync(process.execPath, [
__filename,
'child'
test
]);
assert.strictEqual(proc.stderr.toString(), '');
assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor');
Expand Down

0 comments on commit 0934abd

Please sign in to comment.