Test and fix AsyncWorker

- Remove MakeCallback overload that defaulted to undefined receiver, because node::MakeCallback requires an object. - Allow the AsyncWorker constructor to optionally specify a receiver object, that is persisted and used as of an async callback. - Persist async errors as strings, because an Error object cannot be created outside of a JS context. - Remove overridable AsyncWorker::WorkComplete() because it wasn't useful and caused confusion. OnOK() and/or OnError() should be (optionally) overridden instead. - Add tests to validate basic success and error scenarios for AsyncWorker. - Also add necessary cast to Object when calling Unwrap.
nodejs · jasongin · May 4, 2017 · Apr 27, 2017 · May 4, 2017 · May 4, 2017
commit 4236269ae9a99c44bd301426ed875fda6d4caa31
diff --git a/napi-inl.h b/napi-inl.h
@@ -1,4 +1,4 @@
-#ifndef SRC_NAPI_INL_H_
+#ifndef SRC_NAPI_INL_H_
 #define SRC_NAPI_INL_H_
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -38,19 +38,15 @@ inline void RegisterModule(napi_env env,
                        Napi::Object(env, module));
   }
   catch (const Error& e) {
-    if (!Napi::Env(env).IsExceptionPending()) {
-      e.ThrowAsJavaScriptException();
-    }
+    e.ThrowAsJavaScriptException();
   }
 }
 
 // For use in JS to C++ callback wrappers to catch any Napi::Error exceptions
 // and rethrow them as JavaScript exceptions before returning from the callback.
 #define NAPI_RETHROW_JS_ERROR(env)               \
   catch (const Error& e) {                       \
-    if (!Napi::Env(env).IsExceptionPending()) {  \
-      e.ThrowAsJavaScriptException();            \
-    }                                            \
+    e.ThrowAsJavaScriptException();              \
     return nullptr;                              \
   }
 
@@ -1215,14 +1211,6 @@ inline Value Function::Call(napi_value recv, const std::vector<napi_value>& args
   return Value(_env, result);
 }
 
-inline Value Function::MakeCallback(const std::initializer_list<napi_value>& args) const {
-  return MakeCallback(Env().Undefined(), args);
-}
-
-inline Value Function::MakeCallback(const std::vector<napi_value>& args) const {
-  return MakeCallback(Env().Undefined(), args);
-}
-
 inline Value Function::MakeCallback(
     napi_value recv, const std::initializer_list<napi_value>& args) const {
   napi_value result;
@@ -1888,16 +1876,6 @@ inline Napi::Value FunctionReference::Call(
   return scope.Escape(Value().Call(recv, args));
 }
 
-inline Napi::Value FunctionReference::MakeCallback(const std::initializer_list<napi_value>& args) const {
-  EscapableHandleScope scope(_env);
-  return scope.Escape(Value().MakeCallback(args));
-}
-
-inline Napi::Value FunctionReference::MakeCallback(const std::vector<napi_value>& args) const {
-  EscapableHandleScope scope(_env);
-  return scope.Escape(Value().MakeCallback(args));
-}
-
 inline Napi::Value FunctionReference::MakeCallback(
     napi_value recv, const std::initializer_list<napi_value>& args) const {
   EscapableHandleScope scope(_env);
@@ -2481,7 +2459,7 @@ inline napi_value ObjectWrap<T>::InstanceVoidMethodCallbackWrapper(
     InstanceVoidMethodCallbackData* callbackData =
       reinterpret_cast<InstanceVoidMethodCallbackData*>(callbackInfo.Data());
     callbackInfo.SetData(callbackData->data);
-    T* instance = Unwrap(callbackInfo.This());
+    T* instance = Unwrap(callbackInfo.This().As<Object>());
     auto cb = callbackData->callback;
     (instance->*cb)(callbackInfo);
     return nullptr;
@@ -2498,7 +2476,7 @@ inline napi_value ObjectWrap<T>::InstanceMethodCallbackWrapper(
     InstanceMethodCallbackData* callbackData =
       reinterpret_cast<InstanceMethodCallbackData*>(callbackInfo.Data());
     callbackInfo.SetData(callbackData->data);
-    T* instance = Unwrap(callbackInfo.This());
+    T* instance = Unwrap(callbackInfo.This().As<Object>());
     auto cb = callbackData->callback;
     return (instance->*cb)(callbackInfo);
   }
@@ -2514,7 +2492,7 @@ inline napi_value ObjectWrap<T>::InstanceGetterCallbackWrapper(
     InstanceAccessorCallbackData* callbackData =
       reinterpret_cast<InstanceAccessorCallbackData*>(callbackInfo.Data());
     callbackInfo.SetData(callbackData->data);
-    T* instance = Unwrap(callbackInfo.This());
+    T* instance = Unwrap(callbackInfo.This().As<Object>());
     auto cb = callbackData->getterCallback;
     return (instance->*cb)(callbackInfo);
   }
@@ -2530,7 +2508,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
     InstanceAccessorCallbackData* callbackData =
       reinterpret_cast<InstanceAccessorCallbackData*>(callbackInfo.Data());
     callbackInfo.SetData(callbackData->data);
-    T* instance = Unwrap(callbackInfo.This());
+    T* instance = Unwrap(callbackInfo.This().As<Object>());
     auto cb = callbackData->setterCallback;
     (instance->*cb)(callbackInfo, callbackInfo[0]);
     return nullptr;
@@ -2606,9 +2584,13 @@ inline Value EscapableHandleScope::Escape(napi_value escapee) {
 ////////////////////////////////////////////////////////////////////////////////
 
 inline AsyncWorker::AsyncWorker(const Function& callback)
-  : _callback(Napi::Persistent(callback)),
-    _persistent(Napi::Persistent(Object::New(callback.Env()))),
-    _env(callback.Env()) {
+  : AsyncWorker(Object::New(callback.Env()), callback) {
+}
+
+inline AsyncWorker::AsyncWorker(const Object& receiver, const Function& callback)
+  : _env(callback.Env()),
+    _receiver(Napi::Persistent(receiver)),
+    _callback(Napi::Persistent(callback)) {
   napi_status status = napi_create_async_work(
     _env, OnExecute, OnWorkComplete, this, &_work);
   if (status != napi_ok) throw Error::New(_env);
@@ -2626,7 +2608,8 @@ inline AsyncWorker::AsyncWorker(AsyncWorker&& other) {
   other._env = nullptr;
   _work = other._work;
   other._work = nullptr;
-  _persistent = std::move(other._persistent);
+  _receiver = std::move(other._receiver);
+  _callback = std::move(other._callback);
   _error = std::move(other._error);
 }
 
@@ -2635,7 +2618,8 @@ inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) {
   other._env = nullptr;
   _work = other._work;
   other._work = nullptr;
-  _persistent = std::move(other._persistent);
+  _receiver = std::move(other._receiver);
+  _callback = std::move(other._callback);
   _error = std::move(other._error);
   return *this;
 }
@@ -2658,48 +2642,59 @@ inline void AsyncWorker::Cancel() {
   if (status != napi_ok) throw Error::New(_env);
 }
 
-inline void AsyncWorker::WorkComplete() {
-  HandleScope scope(_env);
-  if (_error.IsEmpty()) {
-    OnOK();
-  }
-  else {
-    OnError(_error);
-  }
+inline ObjectReference& AsyncWorker::Receiver() {
+  return _receiver;
 }
 
-inline ObjectReference& AsyncWorker::Persistent() {
-  return _persistent;
+inline FunctionReference& AsyncWorker::Callback() {
+  return _callback;
 }
 
 inline void AsyncWorker::OnOK() {
-  _callback.MakeCallback(Env().Undefined(), std::vector<napi_value>());
+  _callback.MakeCallback(_receiver.Value(), {});
 }
 
-inline void AsyncWorker::OnError(Error e) {
-  HandleScope scope(Env());
-  _callback.MakeCallback(Env().Undefined(), std::vector<napi_value>({ e.Value() }));
+inline void AsyncWorker::OnError(Error& e) {
+  _callback.MakeCallback(_receiver.Value(), { e.Value() });
 }
 
-inline void AsyncWorker::SetError(Error error) {
+inline void AsyncWorker::SetError(const std::string& error) {
   _error = error;
 }
 
 inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
   AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
   try {
     self->Execute();
-  }
-  catch (const Error& e) {
-    self->SetError(e);
+  } catch (const std::exception& e) {
+    self->SetError(e.what());
   }
 }
 
 inline void AsyncWorker::OnWorkComplete(
     napi_env env, napi_status status, void* this_pointer) {
   AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
   if (status != napi_cancelled) {
-    self->WorkComplete();
+    HandleScope scope(self->_env);
+    try {
+      if (self->_error.size() == 0) {
+        self->OnOK();
+      }
+      else {
+        self->OnError(Error::New(self->_env, self->_error));
+      }
+    } catch (const Error& e) {
+      // A JS or N-API exception was thrown, and propagated as a C++ exception.
+      // But throwing a JS exception here would have no effect because there
+      // is no JS on the callstack.
+
+      // TODO: Print the exception details and abort, just like Node.js normally
+      // does for unhandled exceptions. But there is no N-API function for that.
+      // For now just assert, so at least the exception message is discoverable
+      // in a debug build.
+      const char* message = e.what();
+      assert(false);
+    }
   }
   delete self;
 }

diff --git a/napi.h b/napi.h
@@ -1,4 +1,4 @@
-#ifndef SRC_NAPI_H_
+#ifndef SRC_NAPI_H_
 #define SRC_NAPI_H_
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -401,8 +401,6 @@ namespace Napi {
     Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
     Value Call(napi_value recv, const std::vector<napi_value>& args) const;
 
-    Value MakeCallback(const std::initializer_list<napi_value>& args) const;
-    Value MakeCallback(const std::vector<napi_value>& args) const;
     Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
     Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
 
@@ -548,8 +546,6 @@ namespace Napi {
     Napi::Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
     Napi::Value Call(napi_value recv, const std::vector<napi_value>& args) const;
 
-    Napi::Value MakeCallback(const std::initializer_list<napi_value>& args) const;
-    Napi::Value MakeCallback(const std::vector<napi_value>& args) const;
     Napi::Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
     Napi::Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
 
@@ -966,21 +962,18 @@ namespace Napi {
     void Queue();
     void Cancel();
 
-    virtual void Execute() = 0;
-    virtual void WorkComplete();
-
-    ObjectReference& Persistent();
+    ObjectReference& Receiver();
+    FunctionReference& Callback();
 
   protected:
     explicit AsyncWorker(const Function& callback);
+    explicit AsyncWorker(const Object& receiver, const Function& callback);
 
+    virtual void Execute() = 0;
     virtual void OnOK();
-    virtual void OnError(Error e);
+    virtual void OnError(Error& e);
 
-    void SetError(Error error);
-
-    FunctionReference _callback;
-    ObjectReference _persistent;
+    void SetError(const std::string& error);
 
   private:
     static void OnExecute(napi_env env, void* this_pointer);
@@ -990,7 +983,9 @@ namespace Napi {
 
     napi_env _env;
     napi_async_work _work;
-    Error _error;
+    ObjectReference _receiver;
+    FunctionReference _callback;
+    std::string _error;
   };
 
 } // namespace Napi

diff --git a/test/asyncworker.cc b/test/asyncworker.cc
@@ -0,0 +1,34 @@
+#include "napi.h"
+
+using namespace Napi;
+
+class TestWorker : public AsyncWorker {
+public:
+  static void DoWork(const CallbackInfo& info) {
+    bool succeed = info[0].As<Boolean>();
+    Function cb = info[1].As<Function>();
+    Value data = info[2];
+
+    TestWorker* worker = new TestWorker(cb);
+    worker->Receiver().Set("data", data);
+    worker->_succeed = succeed;
+    worker->Queue();
+  }
+
+protected:
+  void Execute() override {
+    if (!_succeed) {
+      SetError("test error");
+    }
+  }
+
+private:
+  TestWorker(Function cb) : AsyncWorker(cb) {}
+  bool _succeed;
+};
+
+Object InitAsyncWorker(Env env) {
+  Object exports = Object::New(env);
+  exports["doWork"] = Function::New(env, TestWorker::DoWork);
+  return exports;
+}
diff --git a/test/asyncworker.js b/test/asyncworker.js
@@ -0,0 +1,25 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const binding = require(`./build/${buildType}/binding.node`);
+const assert = require('assert');
+
+// Use setTimeout() when asserting after async callbacks because
+// unhandled JS exceptions in async callbacks are currently ignored.
+// See the TODO comment in AsyncWorker::OnWorkComplete().
+
+binding.asyncworker.doWork(true, function (e) {
+  setTimeout(() => {
+    assert.strictEqual(typeof e, 'undefined');
+    assert.strictEqual(typeof this, 'object');
+    assert.strictEqual(this.data, 'test data');
+  });
+}, 'test data');
+
+binding.asyncworker.doWork(false, function (e) {
+  setTimeout(() => {
+    assert.ok(e instanceof Error);
+    assert.strictEqual(e.message, 'test error');
+    assert.strictEqual(typeof this, 'object');
+    assert.strictEqual(this.data, 'test data');
+  });
+}, 'test data');
diff --git a/test/binding.cc b/test/binding.cc
@@ -3,6 +3,7 @@
 using namespace Napi;
 
 Object InitArrayBuffer(Env env);
+Object InitAsyncWorker(Env env);
 Object InitBuffer(Env env);
 Object InitError(Env env);
 Object InitExternal(Env env);
@@ -12,6 +13,7 @@ Object InitObject(Env env);
 
 void Init(Env env, Object exports, Object module) {
   exports.Set("arraybuffer", InitArrayBuffer(env));
+  exports.Set("asyncworker", InitAsyncWorker(env));
   exports.Set("buffer", InitBuffer(env));
   exports.Set("error", InitError(env));
   exports.Set("external", InitExternal(env));

diff --git a/test/binding.gyp b/test/binding.gyp
@@ -4,6 +4,7 @@
       'target_name': 'binding',
       'sources': [
         'arraybuffer.cc',
+        'asyncworker.cc',
         'binding.cc',
         'buffer.cc',
         'error.cc',

diff --git a/test/index.js b/test/index.js
@@ -6,6 +6,7 @@ if (typeof global.gc !== 'function') {
 
 let testModules = [
    'arraybuffer',
+   'asyncworker',
    'buffer',
    'error',
    'external',