From 0934abdb28ccdeac13ac00ce0ea33941340bbb2c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 19 Jan 2019 00:24:20 +0100 Subject: [PATCH 1/2] src: restrict unloading addons to Worker threads 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 https://github.com/nodejs/node/pull/23319. Refs: https://github.com/nodejs/node/pull/24861 Refs: https://github.com/nodejs/node/pull/23319 --- src/env.cc | 8 +++++--- test/addons/worker-addon/binding.cc | 11 +++++++++++ test/addons/worker-addon/test.js | 13 ++++++++++--- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/env.cc b/src/env.cc index 2cbbd14e713b9b..6b4b458e738d5f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -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(); + } } } diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index 1fb85ae230eb5f..7e32a58175d33a 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -3,6 +3,7 @@ #include #include #include +#include using v8::Context; using v8::HandleScope; @@ -41,6 +42,16 @@ void Initialize(Local exports, const_cast(static_cast("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(&extra_async)); + } } NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize) diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js index b6d962c0868741..7fb7c1a87aa903 100644 --- a/test/addons/worker-addon/test.js +++ b/test/addons/worker-addon/test.js @@ -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'); From d7d409ad6eae7a5f332b7fd4ad2bd0d1912c7b1b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 22 Jan 2019 23:13:19 +0100 Subject: [PATCH 2/2] fixup! src: restrict unloading addons to Worker threads --- src/env.cc | 5 +++++ test/addons/worker-addon/binding.cc | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index 6b4b458e738d5f..f45386dce4a058 100644 --- a/src/env.cc +++ b/src/env.cc @@ -276,6 +276,11 @@ Environment::~Environment() { TRACE_EVENT_NESTABLE_ASYNC_END0( TRACING_CATEGORY_NODE1(environment), "Environment", this); + // Do not unload addons on the main thread. Some addons need to retain memory + // beyond the Environment's lifetime, and unloading them early would break + // them; with Worker threads, we have the opportunity to be stricter. + // Also, since the main thread usually stops just before the process exits, + // this is far less relevant here. if (!is_main_thread()) { // Dereference all addons that were loaded into this environment. for (binding::DLib& addon : loaded_addons_) { diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index 7e32a58175d33a..faf3ba8647e5bc 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -49,7 +49,8 @@ void Initialize(Local exports, // 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*) {}); + int err = uv_async_init(loop, &extra_async, [](uv_async_t*) {}); + assert(err == 0); uv_unref(reinterpret_cast(&extra_async)); } }