From 76790681e9f0c6e309fa7adf53b91a6333e32790 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 7 Dec 2018 13:20:24 -0800 Subject: [PATCH] src: add AsyncWorker destruction suppression Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an instance of the class to remain allocated even after the `OnOK` callback fires. Such an instance must be explicitly `delete`-ed from user code. Re: https://github.com/nodejs/node-addon-api/issues/231 Re: https://github.com/nodejs/abi-stable-node/issues/353 --- doc/async_worker.md | 9 +++++ napi-inl.h | 13 +++++-- napi.h | 2 ++ test/asyncworker-persistent.cc | 65 ++++++++++++++++++++++++++++++++++ test/asyncworker-persistent.js | 25 +++++++++++++ test/binding.cc | 2 ++ test/binding.gyp | 7 ++++ test/index.js | 1 + 8 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 test/asyncworker-persistent.cc create mode 100644 test/asyncworker-persistent.js diff --git a/doc/async_worker.md b/doc/async_worker.md index a71b29d9f..43e11d587 100644 --- a/doc/async_worker.md +++ b/doc/async_worker.md @@ -66,6 +66,15 @@ the computation that happened in the `Napi::AsyncWorker::Execute` method, unless the default implementation of `Napi::AsyncWorker::OnOK` or `Napi::AsyncWorker::OnError` is overridden. +### SuppressDestruct + +```cpp +void Napi::AsyncWorker::SuppressDestruct(); +``` + +Prevents the destruction of the `Napi::AsyncWorker` instance upon completion of +the `Napi::AsyncWorker::OnOK` callback. + ### SetError Sets the error message for the error that happened during the execution. Setting diff --git a/napi-inl.h b/napi-inl.h index b831f34c9..35155bda1 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3524,7 +3524,8 @@ inline AsyncWorker::AsyncWorker(const Object& receiver, const Object& resource) : _env(callback.Env()), _receiver(Napi::Persistent(receiver)), - _callback(Napi::Persistent(callback)) { + _callback(Napi::Persistent(callback)), + _suppress_destruct(false) { napi_value resource_id; napi_status status = napi_create_string_latin1( _env, resource_name, NAPI_AUTO_LENGTH, &resource_id); @@ -3550,6 +3551,7 @@ inline AsyncWorker::AsyncWorker(AsyncWorker&& other) { _receiver = std::move(other._receiver); _callback = std::move(other._callback); _error = std::move(other._error); + _suppress_destruct = other._suppress_destruct; } inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) { @@ -3560,6 +3562,7 @@ inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) { _receiver = std::move(other._receiver); _callback = std::move(other._callback); _error = std::move(other._error); + _suppress_destruct = other._suppress_destruct; return *this; } @@ -3589,6 +3592,10 @@ inline FunctionReference& AsyncWorker::Callback() { return _callback; } +inline void AsyncWorker::SuppressDestruct() { + _suppress_destruct = true; +} + inline void AsyncWorker::OnOK() { _callback.Call(_receiver.Value(), std::initializer_list{}); } @@ -3629,7 +3636,9 @@ inline void AsyncWorker::OnWorkComplete( return nullptr; }); } - delete self; + if (!self->_suppress_destruct) { + delete self; + } } //////////////////////////////////////////////////////////////////////////////// diff --git a/napi.h b/napi.h index 16d09943a..2262cf719 100644 --- a/napi.h +++ b/napi.h @@ -1722,6 +1722,7 @@ namespace Napi { void Queue(); void Cancel(); + void SuppressDestruct(); ObjectReference& Receiver(); FunctionReference& Callback(); @@ -1760,6 +1761,7 @@ namespace Napi { ObjectReference _receiver; FunctionReference _callback; std::string _error; + bool _suppress_destruct; }; // Memory management. diff --git a/test/asyncworker-persistent.cc b/test/asyncworker-persistent.cc new file mode 100644 index 000000000..b9c729ef1 --- /dev/null +++ b/test/asyncworker-persistent.cc @@ -0,0 +1,65 @@ +#include "napi.h" + +// A variant of TestWorker wherein destruction is suppressed. That is, instances +// are not destroyed during the `OnOK` callback. They must be explicitly +// destroyed. + +using namespace Napi; + +namespace { + +class PersistentTestWorker : public AsyncWorker { +public: + static PersistentTestWorker* current_worker; + static void DoWork(const CallbackInfo& info) { + bool succeed = info[0].As(); + Function cb = info[1].As(); + + PersistentTestWorker* worker = new PersistentTestWorker(cb, "TestResource"); + current_worker = worker; + + worker->SuppressDestruct(); + worker->_succeed = succeed; + worker->Queue(); + } + + static Value WorkerGone(const CallbackInfo& info) { + return Boolean::New(info.Env(), current_worker == nullptr); + } + + static void DeleteWorker(const CallbackInfo& info) { + (void) info; + delete current_worker; + } + + ~PersistentTestWorker() { + current_worker = nullptr; + } + +protected: + void Execute() override { + if (!_succeed) { + SetError("test error"); + } + } + +private: + PersistentTestWorker(Function cb, + const char* resource_name) + : AsyncWorker(cb, resource_name) {} + + bool _succeed; +}; + +PersistentTestWorker* PersistentTestWorker::current_worker = nullptr; + +} // end of anonymous namespace + +Object InitPersistentAsyncWorker(Env env) { + Object exports = Object::New(env); + exports["doWork"] = Function::New(env, PersistentTestWorker::DoWork); + exports["workerGone"] = Function::New(env, PersistentTestWorker::WorkerGone); + exports["deleteWorker"] = + Function::New(env, PersistentTestWorker::DeleteWorker); + return exports; +} diff --git a/test/asyncworker-persistent.js b/test/asyncworker-persistent.js new file mode 100644 index 000000000..26f3bed52 --- /dev/null +++ b/test/asyncworker-persistent.js @@ -0,0 +1,25 @@ +'use strict'; +const buildType = process.config.target_defaults.default_configuration; +const assert = require('assert'); +const common = require('./common'); +const binding = require(`./build/${buildType}/binding.node`); +const noexceptBinding = require(`./build/${buildType}/binding_noexcept.node`); + +function test(binding, succeed) { + return new Promise((resolve) => + // Can't pass an arrow function to doWork because that results in an + // undefined context inside its body when the function gets called. + binding.doWork(succeed, function() { + setImmediate(() => { + assert.strictEqual(binding.workerGone(this), false); + binding.deleteWorker(this); + assert.strictEqual(binding.workerGone(this), true); + resolve(); + }); + })); +} + +test(binding.persistentasyncworker, false, 'binding') + .then(() => test(binding.persistentasyncworker, true, 'binding')) + .then(() => test(noexceptBinding.persistentasyncworker, false, 'noxbinding')) + .then(() => test(noexceptBinding.persistentasyncworker, true, 'noxbinding')); diff --git a/test/binding.cc b/test/binding.cc index 0068e7480..4a5fec15c 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -6,6 +6,7 @@ using namespace Napi; Object InitArrayBuffer(Env env); Object InitAsyncContext(Env env); Object InitAsyncWorker(Env env); +Object InitPersistentAsyncWorker(Env env); Object InitBasicTypesArray(Env env); Object InitBasicTypesBoolean(Env env); Object InitBasicTypesNumber(Env env); @@ -42,6 +43,7 @@ Object Init(Env env, Object exports) { exports.Set("arraybuffer", InitArrayBuffer(env)); exports.Set("asynccontext", InitAsyncContext(env)); exports.Set("asyncworker", InitAsyncWorker(env)); + exports.Set("persistentasyncworker", InitPersistentAsyncWorker(env)); exports.Set("basic_types_array", InitBasicTypesArray(env)); exports.Set("basic_types_boolean", InitBasicTypesBoolean(env)); exports.Set("basic_types_number", InitBasicTypesNumber(env)); diff --git a/test/binding.gyp b/test/binding.gyp index 77c388074..f2ec2e048 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -8,6 +8,7 @@ 'arraybuffer.cc', 'asynccontext.cc', 'asyncworker.cc', + 'asyncworker-persistent.cc', 'basic_types/array.cc', 'basic_types/boolean.cc', 'basic_types/number.cc', @@ -43,6 +44,12 @@ 'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED'] }, { 'sources': ['object/object_deprecated.cc'] + }], + ['OS!="windows"', { + 'cflags+': ['-fvisibility=hidden'], + 'xcode_settings': { + 'OTHER_CFLAGS': ['-fvisibility=hidden'] + } }] ], 'include_dirs': ["