From ab421b9108fe61ff7cfdfa201b34c0b3c1cb6920 Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Tue, 16 Jul 2019 21:12:14 +0200 Subject: [PATCH 1/2] AsyncWorker: add GetResult() method Adds an overridable `GetResult()` method, providing arguments to the callback invoked in `OnOK()`. Re: https://github.com/nodejs/node-addon-api/issues/231#issuecomment-511504055 --- doc/async_worker.md | 14 ++++++++++++-- napi-inl.h | 10 ++++++++-- napi.h | 1 + test/asyncworker.cc | 12 ++++++++++++ test/asyncworker.js | 43 +++++++++++++++++++++++++++++++++++++++---- 5 files changed, 72 insertions(+), 8 deletions(-) diff --git a/doc/async_worker.md b/doc/async_worker.md index 0516af8f6..76c2450aa 100644 --- a/doc/async_worker.md +++ b/doc/async_worker.md @@ -79,7 +79,7 @@ the `Napi::AsyncWorker::OnOK` callback. Sets the error message for the error that happened during the execution. Setting an error message will cause the `Napi::AsyncWorker::OnError` method to be -invoked instead of `Napi::AsyncWorker::OnOKOnOK` once the +invoked instead of `Napi::AsyncWorker::OnOK` once the `Napi::AsyncWorker::Execute` method completes. ```cpp @@ -107,11 +107,21 @@ virtual void Napi::AsyncWorker::Execute() = 0; This method is invoked when the computation in the `Execute` method ends. The default implementation runs the Callback optionally provided when the AsyncWorker class -was created. +was created. The callback will by default receive no arguments. To provide arguments, +override the `GetResult()` method. ```cpp virtual void Napi::AsyncWorker::OnOK(); ``` +### GetResult + +This method returns the arguments passed to the Callback invoked by the default +`OnOK()` implementation. The default implementation returns an empty vector, +providing no arguments to the Callback. + +```cpp +virtual std::vector Napi::AsyncWorker::GetResult(Napi::Env env); +``` ### OnError diff --git a/napi-inl.h b/napi-inl.h index 0db3c9830..166cbb1e3 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3686,7 +3686,7 @@ inline void AsyncWorker::SuppressDestruct() { inline void AsyncWorker::OnOK() { if (!_callback.IsEmpty()) { - _callback.Call(_receiver.Value(), std::initializer_list{}); + _callback.Call(_receiver.Value(), GetResult(_callback.Env())); } } @@ -3700,7 +3700,13 @@ inline void AsyncWorker::SetError(const std::string& error) { _error = error; } -inline void AsyncWorker::OnExecute(napi_env /*env*/, void* this_pointer) { +inline std::vector AsyncWorker::GetResult(Napi::Env env) { + return {}; +} +// The OnExecute method receives an napi_env argument. However, do NOT +// use it within this method, as it does not run on the main thread and cannot +// access the napi_env. +inline void AsyncWorker::OnExecute(napi_env /*DO_NOT_USE*/, void* this_pointer) { AsyncWorker* self = static_cast(this_pointer); #ifdef NAPI_CPP_EXCEPTIONS try { diff --git a/napi.h b/napi.h index c1946413b..d772fd50d 100644 --- a/napi.h +++ b/napi.h @@ -1812,6 +1812,7 @@ namespace Napi { virtual void OnOK(); virtual void OnError(const Error& e); virtual void Destroy(); + virtual std::vector GetResult(Napi::Env env); void SetError(const std::string& error); diff --git a/test/asyncworker.cc b/test/asyncworker.cc index bbd7e0b19..105db851f 100644 --- a/test/asyncworker.cc +++ b/test/asyncworker.cc @@ -9,14 +9,25 @@ class TestWorker : public AsyncWorker { Object resource = info[1].As(); Function cb = info[2].As(); Value data = info[3]; + bool setResult = info[4].As(); TestWorker* worker = new TestWorker(cb, "TestResource", resource); worker->Receiver().Set("data", data); worker->_succeed = succeed; + worker->_setResult = setResult; worker->Queue(); } protected: + std::vector GetResult(Napi::Env env) override { + if (_setResult) + return {Boolean::New(env, _succeed), + String::New(env, _succeed ? "ok" : "error")}; + else { + return {}; + } + } + void Execute() override { if (!_succeed) { SetError("test error"); @@ -27,6 +38,7 @@ class TestWorker : public AsyncWorker { TestWorker(Function cb, const char* resource_name, const Object& resource) : AsyncWorker(cb, resource_name, resource) {} bool _succeed; + bool _setResult; }; class TestWorkerNoCallback : public AsyncWorker { diff --git a/test/asyncworker.js b/test/asyncworker.js index 676afd537..d345d0b79 100644 --- a/test/asyncworker.js +++ b/test/asyncworker.js @@ -58,14 +58,24 @@ function test(binding) { assert.strictEqual(typeof e, 'undefined'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data'); + }, 'test data', false); + + binding.asyncworker.doWork(true, {}, function (succeed, succeedString) { + assert(arguments.length == 2); + assert(succeed); + assert(succeedString == "ok"); + assert.strictEqual(typeof e, 'undefined'); + assert.strictEqual(typeof this, 'object'); + assert.strictEqual(this.data, 'test data'); + console.log("ok!"); + }, 'test data', true); binding.asyncworker.doWork(false, {}, function (e) { assert.ok(e instanceof Error); assert.strictEqual(e.message, 'test error'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data'); + }, 'test data', false); return; } @@ -76,7 +86,32 @@ function test(binding) { assert.strictEqual(typeof e, 'undefined'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data'); + }, 'test data', false); + + hooks.then(actual => { + assert.deepStrictEqual(actual, [ + { eventName: 'init', + type: 'TestResource', + triggerAsyncId: triggerAsyncId, + resource: { foo: 'foo' } }, + { eventName: 'before' }, + { eventName: 'after' }, + { eventName: 'destroy' } + ]); + }).catch(common.mustNotCall()); + } + + + { + const hooks = installAsyncHooksForTest(); + const triggerAsyncId = async_hooks.executionAsyncId(); + binding.asyncworker.doWork(true, { foo: 'foo' }, function (succeed, succeedString) { + assert(arguments.length == 2); + assert(succeed); + assert(succeedString == "ok"); + assert.strictEqual(typeof this, 'object'); + assert.strictEqual(this.data, 'test data'); + }, 'test data', true); hooks.then(actual => { assert.deepStrictEqual(actual, [ @@ -100,7 +135,7 @@ function test(binding) { assert.strictEqual(e.message, 'test error'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data'); + }, 'test data', false); hooks.then(actual => { assert.deepStrictEqual(actual, [ From 1091d9372efc3f0396e7e7593cd647fcf38937cb Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Wed, 17 Jul 2019 18:27:09 +0200 Subject: [PATCH 2/2] test: rework AsyncWorker GetResult tests --- napi-inl.h | 5 +++-- test/asyncworker.cc | 41 +++++++++++++++++++++++++++++++---------- test/asyncworker.js | 31 ++++++++++++++----------------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 166cbb1e3..1a47d5084 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3704,8 +3704,9 @@ inline std::vector AsyncWorker::GetResult(Napi::Env env) { return {}; } // The OnExecute method receives an napi_env argument. However, do NOT -// use it within this method, as it does not run on the main thread and cannot -// access the napi_env. +// use it within this method, as it does not run on the main thread and must +// not run any method that would cause JavaScript to run. In practice, this +// means that almost any use of napi_env will be incorrect. inline void AsyncWorker::OnExecute(napi_env /*DO_NOT_USE*/, void* this_pointer) { AsyncWorker* self = static_cast(this_pointer); #ifdef NAPI_CPP_EXCEPTIONS diff --git a/test/asyncworker.cc b/test/asyncworker.cc index 105db851f..324146533 100644 --- a/test/asyncworker.cc +++ b/test/asyncworker.cc @@ -9,36 +9,56 @@ class TestWorker : public AsyncWorker { Object resource = info[1].As(); Function cb = info[2].As(); Value data = info[3]; - bool setResult = info[4].As(); TestWorker* worker = new TestWorker(cb, "TestResource", resource); worker->Receiver().Set("data", data); worker->_succeed = succeed; - worker->_setResult = setResult; worker->Queue(); } protected: - std::vector GetResult(Napi::Env env) override { - if (_setResult) - return {Boolean::New(env, _succeed), - String::New(env, _succeed ? "ok" : "error")}; - else { - return {}; + void Execute() override { + if (!_succeed) { + SetError("test error"); } } +private: + TestWorker(Function cb, const char* resource_name, const Object& resource) + : AsyncWorker(cb, resource_name, resource) {} + bool _succeed; +}; + +class TestWorkerWithResult : public AsyncWorker { +public: + static void DoWork(const CallbackInfo& info) { + bool succeed = info[0].As(); + Object resource = info[1].As(); + Function cb = info[2].As(); + Value data = info[3]; + + TestWorkerWithResult* worker = new TestWorkerWithResult(cb, "TestResource", resource); + worker->Receiver().Set("data", data); + worker->_succeed = succeed; + worker->Queue(); + } + +protected: void Execute() override { if (!_succeed) { SetError("test error"); } } + std::vector GetResult(Napi::Env env) override { + return {Boolean::New(env, _succeed), + String::New(env, _succeed ? "ok" : "error")}; + } + private: - TestWorker(Function cb, const char* resource_name, const Object& resource) + TestWorkerWithResult(Function cb, const char* resource_name, const Object& resource) : AsyncWorker(cb, resource_name, resource) {} bool _succeed; - bool _setResult; }; class TestWorkerNoCallback : public AsyncWorker { @@ -77,5 +97,6 @@ Object InitAsyncWorker(Env env) { Object exports = Object::New(env); exports["doWork"] = Function::New(env, TestWorker::DoWork); exports["doWorkNoCallback"] = Function::New(env, TestWorkerNoCallback::DoWork); + exports["doWorkWithResult"] = Function::New(env, TestWorkerWithResult::DoWork); return exports; } diff --git a/test/asyncworker.js b/test/asyncworker.js index d345d0b79..04415d522 100644 --- a/test/asyncworker.js +++ b/test/asyncworker.js @@ -58,24 +58,22 @@ function test(binding) { assert.strictEqual(typeof e, 'undefined'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data', false); - - binding.asyncworker.doWork(true, {}, function (succeed, succeedString) { - assert(arguments.length == 2); - assert(succeed); - assert(succeedString == "ok"); - assert.strictEqual(typeof e, 'undefined'); - assert.strictEqual(typeof this, 'object'); - assert.strictEqual(this.data, 'test data'); - console.log("ok!"); - }, 'test data', true); + }, 'test data'); binding.asyncworker.doWork(false, {}, function (e) { assert.ok(e instanceof Error); assert.strictEqual(e.message, 'test error'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data', false); + }, 'test data'); + + binding.asyncworker.doWorkWithResult(true, {}, function (succeed, succeedString) { + assert(arguments.length == 2); + assert(succeed); + assert(succeedString == "ok"); + assert.strictEqual(typeof this, 'object'); + assert.strictEqual(this.data, 'test data'); + }, 'test data'); return; } @@ -86,7 +84,7 @@ function test(binding) { assert.strictEqual(typeof e, 'undefined'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data', false); + }, 'test data'); hooks.then(actual => { assert.deepStrictEqual(actual, [ @@ -101,17 +99,16 @@ function test(binding) { }).catch(common.mustNotCall()); } - { const hooks = installAsyncHooksForTest(); const triggerAsyncId = async_hooks.executionAsyncId(); - binding.asyncworker.doWork(true, { foo: 'foo' }, function (succeed, succeedString) { + binding.asyncworker.doWorkWithResult(true, { foo: 'foo' }, function (succeed, succeedString) { assert(arguments.length == 2); assert(succeed); assert(succeedString == "ok"); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data', true); + }, 'test data'); hooks.then(actual => { assert.deepStrictEqual(actual, [ @@ -135,7 +132,7 @@ function test(binding) { assert.strictEqual(e.message, 'test error'); assert.strictEqual(typeof this, 'object'); assert.strictEqual(this.data, 'test data'); - }, 'test data', false); + }, 'test data'); hooks.then(actual => { assert.deepStrictEqual(actual, [