[WIP][do not merge] Illustrate osx crash

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

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

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.

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);
@@ -39,41 +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("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));
+  exports.Set("persistentasyncworker", InitPersistentAsyncWorker(env));
   return exports;
 }
 

test/binding.gyp

@@ -5,37 +5,8 @@
   },
   'target_defaults': {
     'sources': [
-        'arraybuffer.cc',
-        'asynccontext.cc',
-        'asyncworker.cc',
-        'basic_types/array.cc',
-        'basic_types/boolean.cc',
-        'basic_types/number.cc',
-        'basic_types/value.cc',
-        'bigint.cc',
+        'asyncworker-persistent.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)'] } ],

test/index.js

@@ -8,57 +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',
-  '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'
+  'asyncworker-persistent',
 ];
 
-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');