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: #231
Re: nodejs/abi-stable-node#353

PR-URL: #407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>

Author: Gabriel Schulhof

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

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<napi_value>{});
 }
@@ -3615,7 +3622,9 @@ inline void AsyncWorker::OnWorkComplete(
       return nullptr;
     });
   }
-  delete self;
+  if (!self->_suppress_destruct) {
+    delete self;
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////

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.

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<Boolean>();
+    Function cb = info[1].As<Function>();
+
+    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;
+}

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));

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));

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': ["<!@(node -p \"require('../').include\")"],

test/index.js

@@ -11,6 +11,7 @@ let testModules = [
   'arraybuffer',
   'asynccontext',
   'asyncworker',
+  'asyncworker-persistent',
   'basic_types/array',
   'basic_types/boolean',
   'basic_types/number',