tsfn: fix crash on releasing tsfn

Refs: #531 PR-URL: #532 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
nodejs · Sep 4, 2019 · 0b4f3a5 · 0b4f3a5
1 parent c3c8814
commit 0b4f3a5
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 3 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -3926,12 +3926,12 @@ inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
 }
 
 inline ThreadSafeFunction::ThreadSafeFunction()
-  : _tsfn(new napi_threadsafe_function(nullptr)) {
+  : _tsfn(new napi_threadsafe_function(nullptr), _d) {
 }
 
 inline ThreadSafeFunction::ThreadSafeFunction(
     napi_threadsafe_function tsfn)
-  : _tsfn(new napi_threadsafe_function(tsfn)) {
+  : _tsfn(new napi_threadsafe_function(tsfn), _d) {
 }
 
 inline ThreadSafeFunction::ThreadSafeFunction(ThreadSafeFunction&& other)

diff --git a/napi.h b/napi.h
@@ -2029,8 +2029,13 @@ namespace Napi {
                        napi_value jsCallback,
                        void* context,
                        void* data);
+    struct Deleter {
+      // napi_threadsafe_function is managed by Node.js, leave it alone.
+      void operator()(napi_threadsafe_function*) const {};
+    };
 
-    std::unique_ptr<napi_threadsafe_function> _tsfn;
+    std::unique_ptr<napi_threadsafe_function, Deleter> _tsfn;
+    Deleter _d;
   };
   #endif
 

diff --git a/test/binding.cc b/test/binding.cc
@@ -34,6 +34,7 @@ Object InitObjectDeprecated(Env env);
 #endif // !NODE_ADDON_API_DISABLE_DEPRECATED
 Object InitPromise(Env env);
 #if (NAPI_VERSION > 3)
+Object InitThreadSafeFunctionPtr(Env env);
 Object InitThreadSafeFunction(Env env);
 #endif
 Object InitTypedArray(Env env);
@@ -75,6 +76,7 @@ Object Init(Env env, Object exports) {
 #endif // !NODE_ADDON_API_DISABLE_DEPRECATED
   exports.Set("promise", InitPromise(env));
 #if (NAPI_VERSION > 3)
+  exports.Set("threadsafe_function_ptr", InitThreadSafeFunctionPtr(env));
   exports.Set("threadsafe_function", InitThreadSafeFunction(env));
 #endif
   exports.Set("typedarray", InitTypedArray(env));

diff --git a/test/binding.gyp b/test/binding.gyp
@@ -32,6 +32,7 @@
         'object/object.cc',
         'object/set_property.cc',
         'promise.cc',
+        'threadsafe_function/threadsafe_function_ptr.cc',
         'threadsafe_function/threadsafe_function.cc',
         'typedarray.cc',
         'objectwrap.cc',

diff --git a/test/index.js b/test/index.js
@@ -36,6 +36,7 @@ let testModules = [
   'object/object_deprecated',
   'object/set_property',
   'promise',
+  'threadsafe_function/threadsafe_function_ptr',
   'threadsafe_function/threadsafe_function',
   'typedarray',
   'typedarray-bigint',
@@ -64,6 +65,7 @@ if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
 
 if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
     (process.env.npm_config_NAPI_VERSION < 4)) {
+  testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ptr'), 1);
   testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function'), 1);
 }
 

diff --git a/test/threadsafe_function/threadsafe_function_ptr.cc b/test/threadsafe_function/threadsafe_function_ptr.cc
@@ -0,0 +1,26 @@
+#include "napi.h"
+
+#if (NAPI_VERSION > 3)
+
+using namespace Napi;
+
+namespace {
+
+static Value Test(const CallbackInfo& info) {
+  Object resource = info[0].As<Object>();
+  Function cb = info[1].As<Function>();
+  ThreadSafeFunction tsfn = ThreadSafeFunction::New(info.Env(), cb, resource, "Test", 1, 1);
+  tsfn.Release();
+  return info.Env().Undefined();
+}
+
+}
+
+Object InitThreadSafeFunctionPtr(Env env) {
+  Object exports = Object::New(env);
+  exports["test"] = Function::New(env, Test);
+
+  return exports;
+}
+
+#endif
diff --git a/test/threadsafe_function/threadsafe_function_ptr.js b/test/threadsafe_function/threadsafe_function_ptr.js
@@ -0,0 +1,10 @@
+'use strict';
+
+const buildType = process.config.target_defaults.default_configuration;
+
+test(require(`../build/${buildType}/binding.node`));
+test(require(`../build/${buildType}/binding_noexcept.node`));
+
+function test(binding) {
+  binding.threadsafe_function_ptr.test({}, () => {});
+}