From 1cdfc874417113c4ddea8aa7497a7e95d77c160b Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 7 Dec 2018 13:20:24 -0800 Subject: [PATCH 1/4] 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 | 27 ++++++++++++++ test/binding.cc | 2 ++ test/binding.gyp | 7 ++++ test/index.js | 1 + 8 files changed, 124 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..9516f40a7 --- /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(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..0e2b7d05c 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': [" Date: Mon, 25 Feb 2019 19:16:47 -0800 Subject: [PATCH 2/4] clean up test --- test/asyncworker-persistent.cc | 6 ++++-- test/asyncworker-persistent.js | 14 +++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/test/asyncworker-persistent.cc b/test/asyncworker-persistent.cc index b9c729ef1..97aa0cab8 100644 --- a/test/asyncworker-persistent.cc +++ b/test/asyncworker-persistent.cc @@ -23,7 +23,7 @@ class PersistentTestWorker : public AsyncWorker { worker->Queue(); } - static Value WorkerGone(const CallbackInfo& info) { + static Value GetWorkerGone(const CallbackInfo& info) { return Boolean::New(info.Env(), current_worker == nullptr); } @@ -58,7 +58,9 @@ PersistentTestWorker* PersistentTestWorker::current_worker = nullptr; Object InitPersistentAsyncWorker(Env env) { Object exports = Object::New(env); exports["doWork"] = Function::New(env, PersistentTestWorker::DoWork); - exports["workerGone"] = Function::New(env, PersistentTestWorker::WorkerGone); + 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 index 9516f40a7..90806ebf9 100644 --- a/test/asyncworker-persistent.js +++ b/test/asyncworker-persistent.js @@ -13,15 +13,15 @@ function test(binding, succeed) { 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(this), false); - binding.deleteWorker(this); - assert.strictEqual(binding.workerGone(this), true); + assert.strictEqual(binding.workerGone, false); + binding.deleteWorker(); + assert.strictEqual(binding.workerGone, 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')); +test(binding.persistentasyncworker, false) + .then(() => test(binding.persistentasyncworker, true)) + .then(() => test(noexceptBinding.persistentasyncworker, false)) + .then(() => test(noexceptBinding.persistentasyncworker, true)); From ce11b60bd49002ac6548536ceb894daea221251a Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 5 Mar 2019 11:00:17 -0800 Subject: [PATCH 3/4] remove hidden visibility --- test/binding.gyp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/binding.gyp b/test/binding.gyp index 0e2b7d05c..7b7cd7a48 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -44,12 +44,6 @@ 'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED'] }, { 'sources': ['object/object_deprecated.cc'] - }], - ['OS=="mac"', { - 'cflags+': ['-fvisibility=hidden'], - 'xcode_settings': { - 'OTHER_CFLAGS': ['-fvisibility=hidden'] - } }] ], 'include_dirs': [" Date: Tue, 12 Feb 2019 13:16:53 -0800 Subject: [PATCH 4/4] restrict build --- test/binding.cc | 35 ---------------------------------- test/binding.gyp | 30 ----------------------------- test/index.js | 49 ------------------------------------------------ 3 files changed, 114 deletions(-) diff --git a/test/binding.cc b/test/binding.cc index 4a5fec15c..421ad010d 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -40,42 +40,7 @@ Object InitVersionManagement(Env env); Object InitThunkingManual(Env env); 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)); - exports.Set("basic_types_value", InitBasicTypesValue(env)); -// currently experimental guard with version of NAPI_VERSION that it is -// released in once it is no longer experimental -#if (NAPI_VERSION > 2147483646) - exports.Set("bigint", InitBigInt(env)); -#endif - exports.Set("buffer", InitBuffer(env)); -#if (NAPI_VERSION > 2) - exports.Set("callbackscope", InitCallbackScope(env)); -#endif - exports.Set("dataview", InitDataView(env)); - exports.Set("dataview_read_write", InitDataView(env)); - exports.Set("dataview_read_write", InitDataViewReadWrite(env)); - exports.Set("error", InitError(env)); - exports.Set("external", InitExternal(env)); - exports.Set("function", InitFunction(env)); - exports.Set("name", InitName(env)); - exports.Set("handlescope", InitHandleScope(env)); - exports.Set("memory_management", InitMemoryManagement(env)); - exports.Set("object", InitObject(env)); -#ifndef NODE_ADDON_API_DISABLE_DEPRECATED - exports.Set("object_deprecated", InitObjectDeprecated(env)); -#endif // !NODE_ADDON_API_DISABLE_DEPRECATED - exports.Set("promise", InitPromise(env)); - exports.Set("typedarray", InitTypedArray(env)); - exports.Set("objectwrap", InitObjectWrap(env)); - exports.Set("objectreference", InitObjectReference(env)); - exports.Set("version_management", InitVersionManagement(env)); - exports.Set("thunking_manual", InitThunkingManual(env)); return exports; } diff --git a/test/binding.gyp b/test/binding.gyp index 7b7cd7a48..62fa94675 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -5,38 +5,8 @@ }, 'target_defaults': { 'sources': [ - 'arraybuffer.cc', - 'asynccontext.cc', - 'asyncworker.cc', 'asyncworker-persistent.cc', - 'basic_types/array.cc', - 'basic_types/boolean.cc', - 'basic_types/number.cc', - 'basic_types/value.cc', - 'bigint.cc', 'binding.cc', - 'buffer.cc', - 'callbackscope.cc', - 'dataview/dataview.cc', - 'dataview/dataview_read_write.cc', - 'error.cc', - 'external.cc', - 'function.cc', - 'handlescope.cc', - 'memory_management.cc', - 'name.cc', - 'object/delete_property.cc', - 'object/get_property.cc', - 'object/has_own_property.cc', - 'object/has_property.cc', - 'object/object.cc', - 'object/set_property.cc', - 'promise.cc', - 'typedarray.cc', - 'objectwrap.cc', - 'objectreference.cc', - 'version_management.cc', - 'thunking_manual.cc', ], 'conditions': [ ['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ], diff --git a/test/index.js b/test/index.js index 67e630d7f..4d85f2e96 100644 --- a/test/index.js +++ b/test/index.js @@ -8,58 +8,9 @@ process.config.target_defaults.default_configuration = // FIXME: We might need a way to load test modules automatically without // explicit declaration as follows. let testModules = [ - 'arraybuffer', - 'asynccontext', - 'asyncworker', 'asyncworker-persistent', - 'basic_types/array', - 'basic_types/boolean', - 'basic_types/number', - 'basic_types/value', - 'bigint', - 'buffer', - 'callbackscope', - 'dataview/dataview', - 'dataview/dataview_read_write', - 'error', - 'external', - 'function', - 'handlescope', - 'memory_management', - 'name', - 'object/delete_property', - 'object/get_property', - 'object/has_own_property', - 'object/has_property', - 'object/object', - 'object/object_deprecated', - 'object/set_property', - 'promise', - 'typedarray', - 'typedarray-bigint', - 'objectwrap', - 'objectreference', - 'version_management' ]; -if ((process.env.npm_config_NAPI_VERSION !== undefined) && - (process.env.npm_config_NAPI_VERSION < 50000)) { - // currently experimental only test if NAPI_VERSION - // is set to experimental. We can't use C max int - // as that is not supported as a number on earlier - // Node.js versions. Once bigint is in a release - // this should be guarded on the napi version - // in which bigint was added. - testModules.splice(testModules.indexOf('bigint'), 1); - testModules.splice(testModules.indexOf('typedarray-bigint'), 1); -} - -if ((process.env.npm_config_NAPI_VERSION !== undefined) && - (process.env.npm_config_NAPI_VERSION < 3)) { - testModules.splice(testModules.indexOf('callbackscope'), 1); - testModules.splice(testModules.indexOf('version_management'), 1); -} - if (typeof global.gc === 'function') { console.log('Starting test suite\n');