Skip to content

Commit 0bab14c

Browse files
committed
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.
1 parent 4a65f76 commit 0bab14c

File tree

7 files changed

+121
-68
lines changed

7 files changed

+121
-68
lines changed

napi-inl.h

Lines changed: 48 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#ifndef SRC_NAPI_INL_H_
1+
#ifndef SRC_NAPI_INL_H_
22
#define SRC_NAPI_INL_H_
33

44
////////////////////////////////////////////////////////////////////////////////
@@ -38,19 +38,15 @@ inline void RegisterModule(napi_env env,
3838
Napi::Object(env, module));
3939
}
4040
catch (const Error& e) {
41-
if (!Napi::Env(env).IsExceptionPending()) {
42-
e.ThrowAsJavaScriptException();
43-
}
41+
e.ThrowAsJavaScriptException();
4442
}
4543
}
4644

4745
// For use in JS to C++ callback wrappers to catch any Napi::Error exceptions
4846
// and rethrow them as JavaScript exceptions before returning from the callback.
4947
#define NAPI_RETHROW_JS_ERROR(env) \
5048
catch (const Error& e) { \
51-
if (!Napi::Env(env).IsExceptionPending()) { \
52-
e.ThrowAsJavaScriptException(); \
53-
} \
49+
e.ThrowAsJavaScriptException(); \
5450
return nullptr; \
5551
}
5652

@@ -1215,14 +1211,6 @@ inline Value Function::Call(napi_value recv, const std::vector<napi_value>& args
12151211
return Value(_env, result);
12161212
}
12171213

1218-
inline Value Function::MakeCallback(const std::initializer_list<napi_value>& args) const {
1219-
return MakeCallback(Env().Undefined(), args);
1220-
}
1221-
1222-
inline Value Function::MakeCallback(const std::vector<napi_value>& args) const {
1223-
return MakeCallback(Env().Undefined(), args);
1224-
}
1225-
12261214
inline Value Function::MakeCallback(
12271215
napi_value recv, const std::initializer_list<napi_value>& args) const {
12281216
napi_value result;
@@ -1888,16 +1876,6 @@ inline Napi::Value FunctionReference::Call(
18881876
return scope.Escape(Value().Call(recv, args));
18891877
}
18901878

1891-
inline Napi::Value FunctionReference::MakeCallback(const std::initializer_list<napi_value>& args) const {
1892-
EscapableHandleScope scope(_env);
1893-
return scope.Escape(Value().MakeCallback(args));
1894-
}
1895-
1896-
inline Napi::Value FunctionReference::MakeCallback(const std::vector<napi_value>& args) const {
1897-
EscapableHandleScope scope(_env);
1898-
return scope.Escape(Value().MakeCallback(args));
1899-
}
1900-
19011879
inline Napi::Value FunctionReference::MakeCallback(
19021880
napi_value recv, const std::initializer_list<napi_value>& args) const {
19031881
EscapableHandleScope scope(_env);
@@ -2481,7 +2459,7 @@ inline napi_value ObjectWrap<T>::InstanceVoidMethodCallbackWrapper(
24812459
InstanceVoidMethodCallbackData* callbackData =
24822460
reinterpret_cast<InstanceVoidMethodCallbackData*>(callbackInfo.Data());
24832461
callbackInfo.SetData(callbackData->data);
2484-
T* instance = Unwrap(callbackInfo.This());
2462+
T* instance = Unwrap(callbackInfo.This().As<Object>());
24852463
auto cb = callbackData->callback;
24862464
(instance->*cb)(callbackInfo);
24872465
return nullptr;
@@ -2498,7 +2476,7 @@ inline napi_value ObjectWrap<T>::InstanceMethodCallbackWrapper(
24982476
InstanceMethodCallbackData* callbackData =
24992477
reinterpret_cast<InstanceMethodCallbackData*>(callbackInfo.Data());
25002478
callbackInfo.SetData(callbackData->data);
2501-
T* instance = Unwrap(callbackInfo.This());
2479+
T* instance = Unwrap(callbackInfo.This().As<Object>());
25022480
auto cb = callbackData->callback;
25032481
return (instance->*cb)(callbackInfo);
25042482
}
@@ -2514,7 +2492,7 @@ inline napi_value ObjectWrap<T>::InstanceGetterCallbackWrapper(
25142492
InstanceAccessorCallbackData* callbackData =
25152493
reinterpret_cast<InstanceAccessorCallbackData*>(callbackInfo.Data());
25162494
callbackInfo.SetData(callbackData->data);
2517-
T* instance = Unwrap(callbackInfo.This());
2495+
T* instance = Unwrap(callbackInfo.This().As<Object>());
25182496
auto cb = callbackData->getterCallback;
25192497
return (instance->*cb)(callbackInfo);
25202498
}
@@ -2530,7 +2508,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
25302508
InstanceAccessorCallbackData* callbackData =
25312509
reinterpret_cast<InstanceAccessorCallbackData*>(callbackInfo.Data());
25322510
callbackInfo.SetData(callbackData->data);
2533-
T* instance = Unwrap(callbackInfo.This());
2511+
T* instance = Unwrap(callbackInfo.This().As<Object>());
25342512
auto cb = callbackData->setterCallback;
25352513
(instance->*cb)(callbackInfo, callbackInfo[0]);
25362514
return nullptr;
@@ -2606,9 +2584,13 @@ inline Value EscapableHandleScope::Escape(napi_value escapee) {
26062584
////////////////////////////////////////////////////////////////////////////////
26072585

26082586
inline AsyncWorker::AsyncWorker(const Function& callback)
2609-
: _callback(Napi::Persistent(callback)),
2610-
_persistent(Napi::Persistent(Object::New(callback.Env()))),
2611-
_env(callback.Env()) {
2587+
: AsyncWorker(Object::New(callback.Env()), callback) {
2588+
}
2589+
2590+
inline AsyncWorker::AsyncWorker(const Object& receiver, const Function& callback)
2591+
: _env(callback.Env()),
2592+
_receiver(Napi::Persistent(receiver)),
2593+
_callback(Napi::Persistent(callback)) {
26122594
napi_status status = napi_create_async_work(
26132595
_env, OnExecute, OnWorkComplete, this, &_work);
26142596
if (status != napi_ok) throw Error::New(_env);
@@ -2626,7 +2608,8 @@ inline AsyncWorker::AsyncWorker(AsyncWorker&& other) {
26262608
other._env = nullptr;
26272609
_work = other._work;
26282610
other._work = nullptr;
2629-
_persistent = std::move(other._persistent);
2611+
_receiver = std::move(other._receiver);
2612+
_callback = std::move(other._callback);
26302613
_error = std::move(other._error);
26312614
}
26322615

@@ -2635,7 +2618,8 @@ inline AsyncWorker& AsyncWorker::operator =(AsyncWorker&& other) {
26352618
other._env = nullptr;
26362619
_work = other._work;
26372620
other._work = nullptr;
2638-
_persistent = std::move(other._persistent);
2621+
_receiver = std::move(other._receiver);
2622+
_callback = std::move(other._callback);
26392623
_error = std::move(other._error);
26402624
return *this;
26412625
}
@@ -2658,48 +2642,59 @@ inline void AsyncWorker::Cancel() {
26582642
if (status != napi_ok) throw Error::New(_env);
26592643
}
26602644

2661-
inline void AsyncWorker::WorkComplete() {
2662-
HandleScope scope(_env);
2663-
if (_error.IsEmpty()) {
2664-
OnOK();
2665-
}
2666-
else {
2667-
OnError(_error);
2668-
}
2645+
inline ObjectReference& AsyncWorker::Receiver() {
2646+
return _receiver;
26692647
}
26702648

2671-
inline ObjectReference& AsyncWorker::Persistent() {
2672-
return _persistent;
2649+
inline FunctionReference& AsyncWorker::Callback() {
2650+
return _callback;
26732651
}
26742652

26752653
inline void AsyncWorker::OnOK() {
2676-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>());
2654+
_callback.MakeCallback(_receiver.Value(), {});
26772655
}
26782656

2679-
inline void AsyncWorker::OnError(Error e) {
2680-
HandleScope scope(Env());
2681-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>({ e.Value() }));
2657+
inline void AsyncWorker::OnError(Error& e) {
2658+
_callback.MakeCallback(_receiver.Value(), { e.Value() });
26822659
}
26832660

2684-
inline void AsyncWorker::SetError(Error error) {
2661+
inline void AsyncWorker::SetError(const std::string& error) {
26852662
_error = error;
26862663
}
26872664

26882665
inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
26892666
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
26902667
try {
26912668
self->Execute();
2692-
}
2693-
catch (const Error& e) {
2694-
self->SetError(e);
2669+
} catch (const std::exception& e) {
2670+
self->SetError(e.what());
26952671
}
26962672
}
26972673

26982674
inline void AsyncWorker::OnWorkComplete(
26992675
napi_env env, napi_status status, void* this_pointer) {
27002676
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
27012677
if (status != napi_cancelled) {
2702-
self->WorkComplete();
2678+
HandleScope scope(self->_env);
2679+
try {
2680+
if (self->_error.size() == 0) {
2681+
self->OnOK();
2682+
}
2683+
else {
2684+
self->OnError(Error::New(self->_env, self->_error));
2685+
}
2686+
} catch (const Error& e) {
2687+
// A JS or N-API exception was thrown, and propagated as a C++ exception.
2688+
// But throwing a JS exception here would have no effect because there
2689+
// is no JS on the callstack.
2690+
2691+
// TODO: Print the exception details and abort, just like Node.js normally
2692+
// does for unhandled exceptions. But there is no N-API function for that.
2693+
// For now just assert, so at least the exception message is discoverable
2694+
// in a debug build.
2695+
const char* message = e.what();
2696+
assert(false);
2697+
}
27032698
}
27042699
delete self;
27052700
}

napi.h

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#ifndef SRC_NAPI_H_
1+
#ifndef SRC_NAPI_H_
22
#define SRC_NAPI_H_
33

44
////////////////////////////////////////////////////////////////////////////////
@@ -401,8 +401,6 @@ namespace Napi {
401401
Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
402402
Value Call(napi_value recv, const std::vector<napi_value>& args) const;
403403

404-
Value MakeCallback(const std::initializer_list<napi_value>& args) const;
405-
Value MakeCallback(const std::vector<napi_value>& args) const;
406404
Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
407405
Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
408406

@@ -548,8 +546,6 @@ namespace Napi {
548546
Napi::Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
549547
Napi::Value Call(napi_value recv, const std::vector<napi_value>& args) const;
550548

551-
Napi::Value MakeCallback(const std::initializer_list<napi_value>& args) const;
552-
Napi::Value MakeCallback(const std::vector<napi_value>& args) const;
553549
Napi::Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
554550
Napi::Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
555551

@@ -966,21 +962,18 @@ namespace Napi {
966962
void Queue();
967963
void Cancel();
968964

969-
virtual void Execute() = 0;
970-
virtual void WorkComplete();
971-
972-
ObjectReference& Persistent();
965+
ObjectReference& Receiver();
966+
FunctionReference& Callback();
973967

974968
protected:
975969
explicit AsyncWorker(const Function& callback);
970+
explicit AsyncWorker(const Object& receiver, const Function& callback);
976971

972+
virtual void Execute() = 0;
977973
virtual void OnOK();
978-
virtual void OnError(Error e);
974+
virtual void OnError(Error& e);
979975

980-
void SetError(Error error);
981-
982-
FunctionReference _callback;
983-
ObjectReference _persistent;
976+
void SetError(const std::string& error);
984977

985978
private:
986979
static void OnExecute(napi_env env, void* this_pointer);
@@ -990,7 +983,9 @@ namespace Napi {
990983

991984
napi_env _env;
992985
napi_async_work _work;
993-
Error _error;
986+
ObjectReference _receiver;
987+
FunctionReference _callback;
988+
std::string _error;
994989
};
995990

996991
} // namespace Napi

test/asyncworker.cc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
#include "napi.h"
2+
3+
using namespace Napi;
4+
5+
class TestWorker : public AsyncWorker {
6+
public:
7+
static void DoWork(const CallbackInfo& info) {
8+
bool succeed = info[0].As<Boolean>();
9+
Function cb = info[1].As<Function>();
10+
Value data = info[2];
11+
12+
TestWorker* worker = new TestWorker(cb);
13+
worker->Receiver().Set("data", data);
14+
worker->_succeed = succeed;
15+
worker->Queue();
16+
}
17+
18+
protected:
19+
void Execute() override {
20+
if (!_succeed) {
21+
SetError("test error");
22+
}
23+
}
24+
25+
private:
26+
TestWorker(Function cb) : AsyncWorker(cb) {}
27+
bool _succeed;
28+
};
29+
30+
Object InitAsyncWorker(Env env) {
31+
Object exports = Object::New(env);
32+
exports["doWork"] = Function::New(env, TestWorker::DoWork);
33+
return exports;
34+
}

test/asyncworker.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
'use strict';
2+
const buildType = process.config.target_defaults.default_configuration;
3+
const binding = require(`./build/${buildType}/binding.node`);
4+
const assert = require('assert');
5+
6+
// Use setTimeout() when asserting after async callbacks because
7+
// unhandled JS exceptions in async callbacks are currently ignored.
8+
// See the TODO comment in AsyncWorker::OnWorkComplete().
9+
10+
binding.asyncworker.doWork(true, function (e) {
11+
setTimeout(() => {
12+
assert.strictEqual(typeof e, 'undefined');
13+
assert.strictEqual(typeof this, 'object');
14+
assert.strictEqual(this.data, 'test data');
15+
});
16+
}, 'test data');
17+
18+
binding.asyncworker.doWork(false, function (e) {
19+
setTimeout(() => {
20+
assert.ok(e instanceof Error);
21+
assert.strictEqual(e.message, 'test error');
22+
assert.strictEqual(typeof this, 'object');
23+
assert.strictEqual(this.data, 'test data');
24+
});
25+
}, 'test data');

test/binding.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using namespace Napi;
44

55
Object InitArrayBuffer(Env env);
6+
Object InitAsyncWorker(Env env);
67
Object InitBuffer(Env env);
78
Object InitError(Env env);
89
Object InitExternal(Env env);
@@ -12,6 +13,7 @@ Object InitObject(Env env);
1213

1314
void Init(Env env, Object exports, Object module) {
1415
exports.Set("arraybuffer", InitArrayBuffer(env));
16+
exports.Set("asyncworker", InitAsyncWorker(env));
1517
exports.Set("buffer", InitBuffer(env));
1618
exports.Set("error", InitError(env));
1719
exports.Set("external", InitExternal(env));

test/binding.gyp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
'target_name': 'binding',
55
'sources': [
66
'arraybuffer.cc',
7+
'asyncworker.cc',
78
'binding.cc',
89
'buffer.cc',
910
'error.cc',

test/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ if (typeof global.gc !== 'function') {
66

77
let testModules = [
88
'arraybuffer',
9+
'asyncworker',
910
'buffer',
1011
'error',
1112
'external',

0 commit comments

Comments
 (0)