Skip to content

Commit 79077cc

Browse files
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 97bc4c1 commit 79077cc

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
@@ -1215,14 +1215,6 @@ inline Value Function::Call(napi_value recv, const std::vector<napi_value>& args
12151215
return Value(_env, result);
12161216
}
12171217

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-
12261218
inline Value Function::MakeCallback(
12271219
napi_value recv, const std::initializer_list<napi_value>& args) const {
12281220
napi_value result;
@@ -1850,16 +1842,6 @@ inline Napi::Value FunctionReference::Call(
18501842
return scope.Escape(Value().Call(recv, args));
18511843
}
18521844

1853-
inline Napi::Value FunctionReference::MakeCallback(const std::initializer_list<napi_value>& args) const {
1854-
EscapableHandleScope scope(_env);
1855-
return scope.Escape(Value().MakeCallback(args));
1856-
}
1857-
1858-
inline Napi::Value FunctionReference::MakeCallback(const std::vector<napi_value>& args) const {
1859-
EscapableHandleScope scope(_env);
1860-
return scope.Escape(Value().MakeCallback(args));
1861-
}
1862-
18631845
inline Napi::Value FunctionReference::MakeCallback(
18641846
napi_value recv, const std::initializer_list<napi_value>& args) const {
18651847
EscapableHandleScope scope(_env);
@@ -2620,47 +2602,38 @@ inline void AsyncWorker::Cancel() {
26202602
if (status != napi_ok) throw Error::New(_env);
26212603
}
26222604

2623-
inline void AsyncWorker::WorkComplete() {
2624-
HandleScope scope(_env);
2625-
if (_error.IsEmpty()) {
2626-
OnOK();
2627-
}
2628-
else {
2629-
OnError(_error.Value());
2630-
}
2631-
}
2632-
26332605
inline ObjectReference& AsyncWorker::Persistent() {
26342606
return _persistent;
26352607
}
26362608

26372609
inline void AsyncWorker::OnOK() {
2638-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>());
2610+
_callback.MakeCallback(_persistent.Value(), {});
26392611
}
26402612

26412613
inline void AsyncWorker::OnError(Error e) {
2642-
_callback.MakeCallback(Env().Undefined(), std::vector<napi_value>({ e }));
2614+
_callback.MakeCallback(_persistent.Value(), { e });
26432615
}
26442616

2645-
inline void AsyncWorker::SetError(Error error) {
2646-
_error.Reset(error, 1);
2617+
inline void AsyncWorker::SetError(std::string error) {
2618+
_error = error;
26472619
}
26482620

26492621
inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
26502622
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
2651-
try {
2652-
self->Execute();
2653-
}
2654-
catch (const Error& e) {
2655-
self->SetError(e);
2656-
}
2623+
self->Execute();
26572624
}
26582625

26592626
inline void AsyncWorker::OnWorkComplete(
26602627
napi_env env, napi_status status, void* this_pointer) {
26612628
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
26622629
if (status != napi_cancelled) {
2663-
self->WorkComplete();
2630+
HandleScope scope(self->_env);
2631+
if (self->_error.size() == 0) {
2632+
self->OnOK();
2633+
}
2634+
else {
2635+
self->OnError(Error::New(self->_env, self->_error));
2636+
}
26642637
}
26652638
delete self;
26662639
}

napi.h

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -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

@@ -641,8 +639,6 @@ namespace Napi {
641639
Napi::Value Call(napi_value recv, const std::initializer_list<napi_value>& args) const;
642640
Napi::Value Call(napi_value recv, const std::vector<napi_value>& args) const;
643641

644-
Napi::Value MakeCallback(const std::initializer_list<napi_value>& args) const;
645-
Napi::Value MakeCallback(const std::vector<napi_value>& args) const;
646642
Napi::Value MakeCallback(napi_value recv, const std::initializer_list<napi_value>& args) const;
647643
Napi::Value MakeCallback(napi_value recv, const std::vector<napi_value>& args) const;
648644

@@ -961,18 +957,16 @@ namespace Napi {
961957
void Queue();
962958
void Cancel();
963959

964-
virtual void Execute() = 0;
965-
virtual void WorkComplete();
966-
967960
ObjectReference& Persistent();
968961

969962
protected:
970963
explicit AsyncWorker(const Function& callback);
971964

965+
virtual void Execute() = 0;
972966
virtual void OnOK();
973967
virtual void OnError(Error e);
974968

975-
void SetError(Error error);
969+
void SetError(std::string error);
976970

977971
FunctionReference _callback;
978972
ObjectReference _persistent;
@@ -985,7 +979,7 @@ namespace Napi {
985979

986980
napi_env _env;
987981
napi_async_work _work;
988-
Reference<Error> _error;
982+
std::string _error;
989983
};
990984

991985
} // 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
@@ -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',

0 commit comments

Comments
 (0)