Skip to content

Commit

Permalink
Test and fix AsyncWorker (#30)
Browse files Browse the repository at this point in the history
 - 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.
  • Loading branch information
jasongin committed May 4, 2017
1 parent 1f1bb5e commit 3066c06
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 68 deletions.
92 changes: 39 additions & 53 deletions napi-inl.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#ifndef SRC_NAPI_INL_H_
#ifndef SRC_NAPI_INL_H_
#define SRC_NAPI_INL_H_

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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; \
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

Expand All @@ -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;
}
Expand All @@ -2658,48 +2642,50 @@ 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) {
e.ThrowAsJavaScriptException();
}
}
delete self;
}
Expand Down
25 changes: 10 additions & 15 deletions napi.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#ifndef SRC_NAPI_H_
#ifndef SRC_NAPI_H_
#define SRC_NAPI_H_

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand All @@ -990,7 +983,9 @@ namespace Napi {

napi_env _env;
napi_async_work _work;
Error _error;
ObjectReference _receiver;
FunctionReference _callback;
std::string _error;
};

} // namespace Napi
Expand Down
34 changes: 34 additions & 0 deletions test/asyncworker.cc
Original file line number Diff line number Diff line change
@@ -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;
}
25 changes: 25 additions & 0 deletions test/asyncworker.js
Original file line number Diff line number Diff line change
@@ -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');
2 changes: 2 additions & 0 deletions test/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
Expand Down
1 change: 1 addition & 0 deletions test/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
'target_name': 'binding',
'sources': [
'arraybuffer.cc',
'asyncworker.cc',
'binding.cc',
'buffer.cc',
'error.cc',
Expand Down
1 change: 1 addition & 0 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ if (typeof global.gc !== 'function') {

let testModules = [
'arraybuffer',
'asyncworker',
'buffer',
'error',
'external',
Expand Down

0 comments on commit 3066c06

Please sign in to comment.