From 52d4929d3d0f1d73854892a39162d13e5cbb5629 Mon Sep 17 00:00:00 2001 From: Marly Fleitas 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 PR-URL: https://github.com/nodejs/node-addon-api/pull/407 Reviewed-By: Michael Dawson Reviewed-By: NickNaso --- doc/async_worker.md | 9 +++++ napi-inl.h | 13 ++++++- napi.h | 2 + test/asyncworker-persistent.cc | 67 ++++++++++++++++++++++++++++++++++ test/asyncworker-persistent.js | 27 ++++++++++++++ test/binding.cc | 2 + test/binding.gyp | 7 ++++ test/index.js | 1 + 8 files changed, 126 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 a71b29d..43e11d5 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 d5fbb1b..c15ed97 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3510,7 +3510,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); @@ -3536,6 +3537,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) { @@ -3546,6 +3548,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; } @@ -3575,6 +3578,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{}); } @@ -3615,7 +3622,9 @@ inline void AsyncWorker::OnWorkComplete( return nullptr; }); } - delete self; + if (!self->_suppress_destruct) { + delete self; + } } //////////////////////////////////////////////////////////////////////////////// diff --git a/napi.h b/napi.h index 13ac485..7a83d79 100644 --- a/napi.h +++ b/napi.h @@ -1778,6 +1778,7 @@ namespace Napi { void Queue(); void Cancel(); + void SuppressDestruct(); ObjectReference& Receiver(); FunctionReference& Callback(); @@ -1816,6 +1817,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 0000000..97aa0ca --- /dev/null +++ b/test/asyncworker-persistent.cc @@ -0,0 +1,67 @@ +#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 GetWorkerGone(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.DefineProperty( + PropertyDescriptor::Accessor(env, exports, "workerGone", + PersistentTestWorker::GetWorkerGone)); + 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 0000000..90806eb --- /dev/null +++ b/test/asyncworker-persistent.js @@ -0,0 +1,27 @@ +'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(e) { + setImmediate(() => { + // If the work is supposed to fail, make sure there's an error. + assert.strictEqual(succeed || e.message === 'test error', true); + assert.strictEqual(binding.workerGone, false); + binding.deleteWorker(); + assert.strictEqual(binding.workerGone, true); + resolve(); + }); + })); +} + +test(binding.persistentasyncworker, false) + .then(() => test(binding.persistentasyncworker, true)) + .then(() => test(noexceptBinding.persistentasyncworker, false)) + .then(() => test(noexceptBinding.persistentasyncworker, true)); diff --git a/test/binding.cc b/test/binding.cc index 0068e74..4a5fec1 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 77c3880..0e2b7d0 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=="mac"', { + 'cflags+': ['-fvisibility=hidden'], + 'xcode_settings': { + 'OTHER_CFLAGS': ['-fvisibility=hidden'] + } }] ], 'include_dirs': ["