Skip to content

Commit 9a0062a

Browse files
jasonginjasongin
committed
Test and fix AsyncWorker
 - Remove MakeCallback overload that defaulted to undefined receiver, because node::MakeCallback requires an object. - Use the async worker persistent object as the receiver of a worker 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
1 parent 7c3b72a commit 9a0062a

File tree

6 files changed

+72
-48
lines changed

6 files changed

+72
-48
lines changed

‎napi-inl.h‎

Lines changed: 12 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,14 +1064,6 @@ inline Value Function::Call(napi_value recv, const std::vector<napi_value>& args
10641064
return Value(_env, result);
10651065
}
10661066

1067-
inline Value Function::MakeCallback(const std::initializer_list<napi_value>& args) const {
1068-
return MakeCallback(Env().Undefined(), args);
1069-
}
1070-
1071-
inline Value Function::MakeCallback(const std::vector<napi_value>& args) const {
1072-
return MakeCallback(Env().Undefined(), args);
1073-
}
1074-
10751067
inline Value Function::MakeCallback(
10761068
napi_value recv, const std::initializer_list<napi_value>& args) const {
10771069
napi_value result;
@@ -1653,16 +1645,6 @@ inline Napi::Value FunctionReference::Call(
16531645
return scope.Escape(Value().Call(recv, args));
16541646
}
16551647

1656-
inline Napi::Value FunctionReference::MakeCallback(const std::initializer_list<napi_value>& args) const {
1657-
EscapableHandleScope scope(_env);
1658-
return scope.Escape(Value().MakeCallback(args));
1659-
}
1660-
1661-
inline Napi::Value FunctionReference::MakeCallback(const std::vector<napi_value>& args) const {
1662-
EscapableHandleScope scope(_env);
1663-
return scope.Escape(Value().MakeCallback(args));
1664-
}
1665-
16661648
inline Napi::Value FunctionReference::MakeCallback(
16671649
napi_value recv, const std::initializer_list<napi_value>& args) const {
16681650
EscapableHandleScope scope(_env);
@@ -2193,47 +2175,38 @@ inline void AsyncWorker::Cancel() {
21932175
if (status != napi_ok) throw Error::New(_env);
21942176
}
21952177

2196-
inline void AsyncWorker::WorkComplete() {
2197-
HandleScope scope(_env);
2198-
if (_error.IsEmpty()) {
2199-
OnOK();
2200-
}
2201-
else {
2202-
OnError(_error.Value());
2203-
}
2204-
}
2205-
22062178
inline ObjectReference& AsyncWorker::Persistent() {
22072179
return _persistent;
22082180
}
22092181

22102182
inline void AsyncWorker::OnOK() {
2211-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>());
2183+
_callback.MakeCallback(_persistent.Value(), {});
22122184
}
22132185

22142186
inline void AsyncWorker::OnError(Error e) {
2215-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>({ e }));
2187+
_callback.MakeCallback(_persistent.Value(), { e });
22162188
}
22172189

2218-
inline void AsyncWorker::SetError(Error error) {
2219-
_error.Reset(error, 1);
2190+
inline void AsyncWorker::SetError(std::string error) {
2191+
_error = error;
22202192
}
22212193

22222194
inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
22232195
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
2224-
try {
2225-
self->Execute();
2226-
}
2227-
catch (const Error& e) {
2228-
self->SetError(e);
2229-
}
2196+
self->Execute();
22302197
}
22312198

22322199
inline void AsyncWorker::OnWorkComplete(
22332200
napi_env env, napi_status status, void* this_pointer) {
22342201
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
22352202
if (status != napi_cancelled) {
2236-
self->WorkComplete();
2203+
HandleScope scope(self->_env);
2204+
if (self->_error.size() == 0) {
2205+
self->OnOK();
2206+
}
2207+
else {
2208+
self->OnError(Error::New(self->_env, self->_error));
2209+
}
22372210
}
22382211
delete self;
22392212
}

‎napi.h‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,6 @@ namespace Napi {
377377
Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
378378
Value Call(napi_value recv, const std::vector<napi_value>& args) const;
379379

380-
Value MakeCallback(const std::initializer_list<napi_value>& args) const;
381-
Value MakeCallback(const std::vector<napi_value>& args) const;
382380
Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
383381
Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
384382

@@ -607,8 +605,6 @@ namespace Napi {
607605
Napi::Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
608606
Napi::Value Call(napi_value recv, const std::vector<napi_value>& args) const;
609607

610-
Napi::Value MakeCallback(const std::initializer_list<napi_value>& args) const;
611-
Napi::Value MakeCallback(const std::vector<napi_value>& args) const;
612608
Napi::Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
613609
Napi::Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
614610

@@ -850,18 +846,16 @@ namespace Napi {
850846
void Queue();
851847
void Cancel();
852848

853-
virtual void Execute() = 0;
854-
virtual void WorkComplete();
855-
856849
ObjectReference& Persistent();
857850

858851
protected:
859852
explicit AsyncWorker(const Function& callback);
860853

854+
virtual void Execute() = 0;
861855
virtual void OnOK();
862856
virtual void OnError(Error e);
863857

864-
void SetError(Error error);
858+
void SetError(std::string error);
865859

866860
FunctionReference _callback;
867861
ObjectReference _persistent;
@@ -874,7 +868,7 @@ namespace Napi {
874868

875869
napi_env _env;
876870
napi_async_work _work;
877-
Reference<Error> _error;
871+
std::string _error;
878872
};
879873

880874
} // namespace Napi

‎test/asyncworker.cc‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
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->_persistent.Set("data", data);
14+
worker->_succeed = succeed;
15+
worker->Queue();
16+
}
17+
18+
protected:
19+
void Execute() override {
20+
if (!_succeed) {
21+
SetError("test");
22+
}
23+
}
24+
25+
void OnOK() override {
26+
Value data = _persistent.Get("data");
27+
_callback.MakeCallback(_persistent.Value(), { Env().Undefined(), data });
28+
}
29+
30+
private:
31+
TestWorker(Function cb) : AsyncWorker(cb) {}
32+
bool _succeed;
33+
};
34+
35+
Object InitAsyncWorker(Env env) {
36+
Object exports = Object::New(env);
37+
exports["doWork"] = Function::New(env, TestWorker::DoWork);
38+
return exports;
39+
}

‎test/asyncworker.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
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+
binding.asyncworker.doWork(true, (e, data) => {
7+
assert.strictEqual(typeof e, 'undefined');
8+
assert.strictEqual(data, 'test');
9+
}, 'test');
10+
11+
binding.asyncworker.doWork(false, (e) => {
12+
assert.strictEqual(typeof e, 'object');
13+
assert.ok(e instanceof Error);
14+
assert.strictEqual(e.message, 'test');
15+
});

‎test/binding.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22

33
using namespace Napi;
44

5+
Object InitAsyncWorker(Env env);
56
Object InitError(Env env);
67
Object InitFunction(Env env);
78
Object InitName(Env env);
89

910
void Init(Env env, Object exports, Object module) {
11+
exports.Set("asyncworker", InitAsyncWorker(env));
1012
exports.Set("error", InitError(env));
1113
exports.Set("function", InitFunction(env));
1214
exports.Set("name", InitName(env));

‎test/binding.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
{
44
'target_name': 'binding',
55
'sources': [
6+
'asyncworker.cc',
67
'binding.cc',
78
'error.cc',
89
'function.cc',

0 commit comments

Comments
 (0)