From 4236269ae9a99c44bd301426ed875fda6d4caa31 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Thu, 27 Apr 2017 16:26:24 -0700 Subject: [PATCH 1/2] 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. --- napi-inl.h | 101 +++++++++++++++++++++----------------------- napi.h | 25 +++++------ test/asyncworker.cc | 34 +++++++++++++++ test/asyncworker.js | 25 +++++++++++ test/binding.cc | 2 + test/binding.gyp | 1 + test/index.js | 1 + 7 files changed, 121 insertions(+), 68 deletions(-) create mode 100644 test/asyncworker.cc create mode 100644 test/asyncworker.js diff --git a/napi-inl.h b/napi-inl.h index 82dae5bea..463304cc8 100644 --- 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,9 +38,7 @@ inline void RegisterModule(napi_env env, Napi::Object(env, module)); } catch (const Error& e) { - if (!Napi::Env(env).IsExceptionPending()) { - e.ThrowAsJavaScriptException(); - } + e.ThrowAsJavaScriptException(); } } @@ -48,9 +46,7 @@ inline void RegisterModule(napi_env env, // 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& args return Value(_env, result); } -inline Value Function::MakeCallback(const std::initializer_list& args) const { - return MakeCallback(Env().Undefined(), args); -} - -inline Value Function::MakeCallback(const std::vector& args) const { - return MakeCallback(Env().Undefined(), args); -} - inline Value Function::MakeCallback( napi_value recv, const std::initializer_list& 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& args) const { - EscapableHandleScope scope(_env); - return scope.Escape(Value().MakeCallback(args)); -} - -inline Napi::Value FunctionReference::MakeCallback(const std::vector& args) const { - EscapableHandleScope scope(_env); - return scope.Escape(Value().MakeCallback(args)); -} - inline Napi::Value FunctionReference::MakeCallback( napi_value recv, const std::initializer_list& args) const { EscapableHandleScope scope(_env); @@ -2481,7 +2459,7 @@ inline napi_value ObjectWrap::InstanceVoidMethodCallbackWrapper( InstanceVoidMethodCallbackData* callbackData = reinterpret_cast(callbackInfo.Data()); callbackInfo.SetData(callbackData->data); - T* instance = Unwrap(callbackInfo.This()); + T* instance = Unwrap(callbackInfo.This().As()); auto cb = callbackData->callback; (instance->*cb)(callbackInfo); return nullptr; @@ -2498,7 +2476,7 @@ inline napi_value ObjectWrap::InstanceMethodCallbackWrapper( InstanceMethodCallbackData* callbackData = reinterpret_cast(callbackInfo.Data()); callbackInfo.SetData(callbackData->data); - T* instance = Unwrap(callbackInfo.This()); + T* instance = Unwrap(callbackInfo.This().As()); auto cb = callbackData->callback; return (instance->*cb)(callbackInfo); } @@ -2514,7 +2492,7 @@ inline napi_value ObjectWrap::InstanceGetterCallbackWrapper( InstanceAccessorCallbackData* callbackData = reinterpret_cast(callbackInfo.Data()); callbackInfo.SetData(callbackData->data); - T* instance = Unwrap(callbackInfo.This()); + T* instance = Unwrap(callbackInfo.This().As()); auto cb = callbackData->getterCallback; return (instance->*cb)(callbackInfo); } @@ -2530,7 +2508,7 @@ inline napi_value ObjectWrap::InstanceSetterCallbackWrapper( InstanceAccessorCallbackData* callbackData = reinterpret_cast(callbackInfo.Data()); callbackInfo.SetData(callbackData->data); - T* instance = Unwrap(callbackInfo.This()); + T* instance = Unwrap(callbackInfo.This().As()); 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,30 +2642,23 @@ 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()); + _callback.MakeCallback(_receiver.Value(), {}); } -inline void AsyncWorker::OnError(Error e) { - HandleScope scope(Env()); - _callback.MakeCallback(Env().Undefined(), std::vector({ 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; } @@ -2689,9 +2666,8 @@ inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) { AsyncWorker* self = static_cast(this_pointer); try { self->Execute(); - } - catch (const Error& e) { - self->SetError(e); + } catch (const std::exception& e) { + self->SetError(e.what()); } } @@ -2699,7 +2675,26 @@ inline void AsyncWorker::OnWorkComplete( napi_env env, napi_status status, void* this_pointer) { AsyncWorker* self = static_cast(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 index 79af620db..d4bce0e17 100644 --- 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& args) const; Value Call(napi_value recv, const std::vector& args) const; - Value MakeCallback(const std::initializer_list& args) const; - Value MakeCallback(const std::vector& args) const; Value MakeCallback(napi_value recv, const std::initializer_list& args) const; Value MakeCallback(napi_value recv, const std::vector& args) const; @@ -548,8 +546,6 @@ namespace Napi { Napi::Value Call(napi_value recv, const std::initializer_list& args) const; Napi::Value Call(napi_value recv, const std::vector& args) const; - Napi::Value MakeCallback(const std::initializer_list& args) const; - Napi::Value MakeCallback(const std::vector& args) const; Napi::Value MakeCallback(napi_value recv, const std::initializer_list& args) const; Napi::Value MakeCallback(napi_value recv, const std::vector& 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 new file mode 100644 index 000000000..0a3c7e211 --- /dev/null +++ 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(); + Function cb = info[1].As(); + 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 new file mode 100644 index 000000000..e43dcc4d0 --- /dev/null +++ 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 index d858e8d66..414719a62 100644 --- 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 index a76e9e51d..38e9443a6 100644 --- 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 index 1ab257561..285d7ce41 100644 --- a/test/index.js +++ b/test/index.js @@ -6,6 +6,7 @@ if (typeof global.gc !== 'function') { let testModules = [ 'arraybuffer', + 'asyncworker', 'buffer', 'error', 'external', From cb46bc5e352fe24061a41ce42ef64b4195346c2d Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Thu, 4 May 2017 15:58:10 -0700 Subject: [PATCH 2/2] Rethrow async cb error as JS exception After https://github.com/nodejs/node/pull/12838 is merged, the exception will be properly reported. --- napi-inl.h | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 463304cc8..58c69078e 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2684,16 +2684,7 @@ inline void AsyncWorker::OnWorkComplete( 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); + e.ThrowAsJavaScriptException(); } } delete self;