From cba26287e95688eb74826b497e0b832254ba95f0 Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 6 Oct 2021 19:00:36 +1100 Subject: [PATCH 1/9] Remove const from `Signal()` on async progress workers --- napi-inl.h | 8 ++++---- napi.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 19aaab447..a816e3bdd 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -5873,12 +5873,12 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { } template -inline void AsyncProgressWorker::Signal() const { +inline void AsyncProgressWorker::Signal() { this->NonBlockingCall(static_cast(nullptr)); } template -inline void AsyncProgressWorker::ExecutionProgress::Signal() const { +inline void AsyncProgressWorker::ExecutionProgress::Signal() { _worker->Signal(); } @@ -5985,7 +5985,7 @@ inline void AsyncProgressQueueWorker::SendProgress_(const T* data, size_t cou } template -inline void AsyncProgressQueueWorker::Signal() const { +inline void AsyncProgressQueueWorker::Signal() { this->NonBlockingCall(nullptr); } @@ -5996,7 +5996,7 @@ inline void AsyncProgressQueueWorker::OnWorkComplete(Napi::Env env, napi_stat } template -inline void AsyncProgressQueueWorker::ExecutionProgress::Signal() const { +inline void AsyncProgressQueueWorker::ExecutionProgress::Signal() { _worker->Signal(); } diff --git a/napi.h b/napi.h index e1ddd0087..d93939dbc 100644 --- a/napi.h +++ b/napi.h @@ -2835,7 +2835,7 @@ namespace Napi { class ExecutionProgress { friend class AsyncProgressWorker; public: - void Signal() const; + void Signal(); void Send(const T* data, size_t count) const; private: explicit ExecutionProgress(AsyncProgressWorker* worker) : _worker(worker) {} @@ -2876,7 +2876,7 @@ namespace Napi { private: void Execute() override; - void Signal() const; + void Signal(); void SendProgress_(const T* data, size_t count); std::mutex _mutex; @@ -2892,7 +2892,7 @@ namespace Napi { class ExecutionProgress { friend class AsyncProgressQueueWorker; public: - void Signal() const; + void Signal(); void Send(const T* data, size_t count) const; private: explicit ExecutionProgress(AsyncProgressQueueWorker* worker) : _worker(worker) {} @@ -2934,7 +2934,7 @@ namespace Napi { private: void Execute() override; - void Signal() const; + void Signal(); void SendProgress_(const T* data, size_t count); }; #endif // NAPI_VERSION > 3 && !defined(__wasm32__) From 7fa874661f56a828f294fa601b275711fe25d8a6 Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 6 Oct 2021 19:08:28 +1100 Subject: [PATCH 2/9] Remove const from ExecutionProgress param of OnProgress() --- napi.h | 4 ++-- test/async_progress_queue_worker.cc | 2 +- test/async_progress_worker.cc | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/napi.h b/napi.h index d93939dbc..aed2769cc 100644 --- a/napi.h +++ b/napi.h @@ -2871,7 +2871,7 @@ namespace Napi { const char* resource_name, const Object& resource); #endif - virtual void Execute(const ExecutionProgress& progress) = 0; + virtual void Execute(ExecutionProgress& progress) = 0; virtual void OnProgress(const T* data, size_t count) = 0; private: @@ -2929,7 +2929,7 @@ namespace Napi { const char* resource_name, const Object& resource); #endif - virtual void Execute(const ExecutionProgress& progress) = 0; + virtual void Execute(ExecutionProgress& progress) = 0; virtual void OnProgress(const T* data, size_t count) = 0; private: diff --git a/test/async_progress_queue_worker.cc b/test/async_progress_queue_worker.cc index eec3f9510..f7eb2c682 100644 --- a/test/async_progress_queue_worker.cc +++ b/test/async_progress_queue_worker.cc @@ -35,7 +35,7 @@ class TestWorker : public AsyncProgressQueueWorker { } protected: - void Execute(const ExecutionProgress& progress) override { + void Execute(ExecutionProgress& progress) override { using namespace std::chrono_literals; std::this_thread::sleep_for(1s); diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 36087e7ca..1a251d032 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -29,7 +29,7 @@ class TestWorker : public AsyncProgressWorker { } protected: - void Execute(const ExecutionProgress& progress) override { + void Execute(ExecutionProgress& progress) override { if (_times < 0) { SetError("test error"); } @@ -77,7 +77,7 @@ class MalignWorker : public AsyncProgressWorker { } protected: - void Execute(const ExecutionProgress& progress) override { + void Execute(ExecutionProgress& progress) override { std::unique_lock lock(_cvm); // Testing a nullptr send is acceptable. progress.Send(nullptr, 0); From 142cbd6edc5c44f67c25bfb20a77313147384b80 Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 6 Oct 2021 19:12:12 +1100 Subject: [PATCH 3/9] Add test for AsyncProgressWorker.Signal() --- test/async_progress_worker.cc | 47 +++++++++++++++++++++++++++++++++++ test/async_progress_worker.js | 21 ++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 1a251d032..a2a2e2185 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -122,12 +122,59 @@ class MalignWorker : public AsyncProgressWorker { std::mutex _cvm; FunctionReference _progress; }; + +class SignalTestWorker : public AsyncProgressWorker { + public: + static void DoWork(const CallbackInfo& info) { + int32_t times = info[0].As().Int32Value(); + Function cb = info[1].As(); + Function progress = info[2].As(); + + SignalTestWorker* worker = new SignalTestWorker( + cb, progress, "TestResource", Object::New(info.Env())); + worker->_times = times; + worker->Queue(); + } + + protected: + void Execute(ExecutionProgress& progress) override { + if (_times < 0) { + SetError("test error"); + } + std::unique_lock lock(_cvm); + for (int32_t idx = 0; idx < _times; idx++) { + progress.Signal(); + _cv.wait(lock); + } + } + + void OnProgress(const ProgressData* /* data */, size_t /* count */) override { + if (!_progress.IsEmpty()) { + _progress.MakeCallback(Receiver().Value(), {}); + } + _cv.notify_one(); + } + + private: + SignalTestWorker(Function cb, + Function progress, + const char* resource_name, + const Object& resource) + : AsyncProgressWorker(cb, resource_name, resource) { + _progress.Reset(progress, 1); + } + std::condition_variable _cv; + std::mutex _cvm; + int32_t _times; + FunctionReference _progress; +}; } // namespace Object InitAsyncProgressWorker(Env env) { Object exports = Object::New(env); exports["doWork"] = Function::New(env, TestWorker::DoWork); exports["doMalignTest"] = Function::New(env, MalignWorker::DoWork); + exports["doSignalTest"] = Function::New(env, SignalTestWorker::DoWork); return exports; } diff --git a/test/async_progress_worker.js b/test/async_progress_worker.js index 2a81b204d..8d9250e73 100644 --- a/test/async_progress_worker.js +++ b/test/async_progress_worker.js @@ -9,6 +9,7 @@ async function test({ asyncprogressworker }) { await success(asyncprogressworker); await fail(asyncprogressworker); await malignTest(asyncprogressworker); + await signalTest(asyncprogressworker); } function success(binding) { @@ -59,3 +60,23 @@ function malignTest(binding) { ); }); } + +function signalTest (binding) { + return new Promise((resolve, reject) => { + const expectedCalls = 3; + let actualCalls = 0; + binding.doWork(expectedCalls, + common.mustCall((err) => { + if (err) { + reject(err); + } + }), + common.mustCall((_progress) => { + actualCalls++; + if (expectedCalls === actualCalls) { + resolve(); + } + }, expectedCalls) + ); + }); +} From 5295dc23034ce8cd5a9f8891d39da996ee590eef Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 6 Oct 2021 19:13:05 +1100 Subject: [PATCH 4/9] Fix test formatting --- test/async_progress_worker.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/async_progress_worker.js b/test/async_progress_worker.js index 8d9250e73..529012528 100644 --- a/test/async_progress_worker.js +++ b/test/async_progress_worker.js @@ -1,18 +1,18 @@ 'use strict'; -const common = require('./common') +const common = require('./common'); const assert = require('assert'); module.exports = common.runTest(test); -async function test({ asyncprogressworker }) { +async function test ({ asyncprogressworker }) { await success(asyncprogressworker); await fail(asyncprogressworker); await malignTest(asyncprogressworker); await signalTest(asyncprogressworker); } -function success(binding) { +function success (binding) { return new Promise((resolve, reject) => { const expected = [0, 1, 2, 3]; const actual = []; @@ -33,11 +33,11 @@ function success(binding) { }); } -function fail(binding) { +function fail (binding) { return new Promise((resolve) => { binding.doWork(-1, common.mustCall((err) => { - assert.throws(() => { throw err }, /test error/) + assert.throws(() => { throw err; }, /test error/); resolve(); }), common.mustNotCall() @@ -45,7 +45,7 @@ function fail(binding) { }); } -function malignTest(binding) { +function malignTest (binding) { return new Promise((resolve, reject) => { binding.doMalignTest( common.mustCall((err) => { From 6074571004eae11b7d5c750f587b4e79a1533288 Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Mon, 11 Oct 2021 16:00:11 +1100 Subject: [PATCH 5/9] Add failing test for `AsyncProgressQueueWorker::Signal()` --- test/async_progress_queue_worker.cc | 53 +++++++++++++++++++++++++++++ test/async_progress_queue_worker.js | 23 +++++++++++++ 2 files changed, 76 insertions(+) diff --git a/test/async_progress_queue_worker.cc b/test/async_progress_queue_worker.cc index f7eb2c682..5064487f4 100644 --- a/test/async_progress_queue_worker.cc +++ b/test/async_progress_queue_worker.cc @@ -71,12 +71,65 @@ class TestWorker : public AsyncProgressQueueWorker { FunctionReference _js_progress_cb; }; +class SignalTestWorker : public AsyncProgressQueueWorker { + public: + static Napi::Value CreateWork(const CallbackInfo& info) { + int32_t times = info[0].As().Int32Value(); + Function cb = info[1].As(); + Function progress = info[2].As(); + + SignalTestWorker* worker = new SignalTestWorker( + cb, progress, "TestResource", Object::New(info.Env()), times); + + return Napi::External::New(info.Env(), worker); + } + + static void QueueWork(const CallbackInfo& info) { + auto wrap = info[0].As>(); + auto worker = wrap.Data(); + worker->Queue(); + } + + protected: + void Execute(ExecutionProgress& progress) override { + using namespace std::chrono_literals; + std::this_thread::sleep_for(1s); + + for (int32_t idx = 0; idx < _times; idx++) { + // TODO: unlike AsyncProgressWorker, this signal does not trigger OnProgress() below, to run + // the JS callback. Investigate and fix. + progress.Signal(); + } + } + + void OnProgress(const ProgressData* /* data */, size_t /* count */) override { + if (!_js_progress_cb.IsEmpty()) { + _js_progress_cb.Call(Receiver().Value(), {}); + } + } + + private: + SignalTestWorker(Function cb, + Function progress, + const char* resource_name, + const Object& resource, + int32_t times) + : AsyncProgressQueueWorker(cb, resource_name, resource), _times(times) { + _js_progress_cb.Reset(progress, 1); + } + + int32_t _times; + FunctionReference _js_progress_cb; +}; + } // namespace Object InitAsyncProgressQueueWorker(Env env) { Object exports = Object::New(env); exports["createWork"] = Function::New(env, TestWorker::CreateWork); exports["queueWork"] = Function::New(env, TestWorker::QueueWork); + exports["createSignalWork"] = Function::New(env, SignalTestWorker::CreateWork); + exports["queueSignalWork"] = Function::New(env, SignalTestWorker::QueueWork); return exports; } diff --git a/test/async_progress_queue_worker.js b/test/async_progress_queue_worker.js index e7150012d..919d2c212 100644 --- a/test/async_progress_queue_worker.js +++ b/test/async_progress_queue_worker.js @@ -8,6 +8,7 @@ module.exports = common.runTest(test); async function test({ asyncprogressqueueworker }) { await success(asyncprogressqueueworker); await fail(asyncprogressqueueworker); + await signalTest(asyncprogressqueueworker); } function success(binding) { @@ -44,3 +45,25 @@ function fail(binding) { binding.queueWork(worker); }); } + +function signalTest(binding) { + return new Promise((resolve, reject) => { + const expectedCalls = 4; + let actualCalls = 0; + const worker = binding.createSignalWork(expectedCalls, + common.mustCall((err) => { + if (err) { + reject(err); + } else { + if (actualCalls === expectedCalls) { + resolve(); + } + } + }), + common.mustCall((_progress) => { + actualCalls++; + }, expectedCalls) + ); + binding.queueSignalWork(worker); + }); +} From ea3dfe3bcfa3165f8bb2b2c0bcb2658f675a3d07 Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Mon, 11 Oct 2021 16:00:18 +1100 Subject: [PATCH 6/9] Fix formatting --- test/async_progress_queue_worker.cc | 15 ++++++++------- test/async_progress_queue_worker.js | 12 ++++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/test/async_progress_queue_worker.cc b/test/async_progress_queue_worker.cc index 5064487f4..4faf6da98 100644 --- a/test/async_progress_queue_worker.cc +++ b/test/async_progress_queue_worker.cc @@ -96,8 +96,8 @@ class SignalTestWorker : public AsyncProgressQueueWorker { std::this_thread::sleep_for(1s); for (int32_t idx = 0; idx < _times; idx++) { - // TODO: unlike AsyncProgressWorker, this signal does not trigger OnProgress() below, to run - // the JS callback. Investigate and fix. + // TODO: unlike AsyncProgressWorker, this signal does not trigger + // OnProgress() below, to run the JS callback. Investigate and fix. progress.Signal(); } } @@ -110,10 +110,10 @@ class SignalTestWorker : public AsyncProgressQueueWorker { private: SignalTestWorker(Function cb, - Function progress, - const char* resource_name, - const Object& resource, - int32_t times) + Function progress, + const char* resource_name, + const Object& resource, + int32_t times) : AsyncProgressQueueWorker(cb, resource_name, resource), _times(times) { _js_progress_cb.Reset(progress, 1); } @@ -128,7 +128,8 @@ Object InitAsyncProgressQueueWorker(Env env) { Object exports = Object::New(env); exports["createWork"] = Function::New(env, TestWorker::CreateWork); exports["queueWork"] = Function::New(env, TestWorker::QueueWork); - exports["createSignalWork"] = Function::New(env, SignalTestWorker::CreateWork); + exports["createSignalWork"] = + Function::New(env, SignalTestWorker::CreateWork); exports["queueSignalWork"] = Function::New(env, SignalTestWorker::QueueWork); return exports; } diff --git a/test/async_progress_queue_worker.js b/test/async_progress_queue_worker.js index 919d2c212..d12bda6d1 100644 --- a/test/async_progress_queue_worker.js +++ b/test/async_progress_queue_worker.js @@ -1,17 +1,17 @@ 'use strict'; -const common = require('./common') +const common = require('./common'); const assert = require('assert'); module.exports = common.runTest(test); -async function test({ asyncprogressqueueworker }) { +async function test ({ asyncprogressqueueworker }) { await success(asyncprogressqueueworker); await fail(asyncprogressqueueworker); await signalTest(asyncprogressqueueworker); } -function success(binding) { +function success (binding) { return new Promise((resolve, reject) => { const expected = [0, 1, 2, 3]; const actual = []; @@ -33,11 +33,11 @@ function success(binding) { }); } -function fail(binding) { +function fail (binding) { return new Promise((resolve, reject) => { const worker = binding.createWork(-1, common.mustCall((err) => { - assert.throws(() => { throw err }, /test error/); + assert.throws(() => { throw err; }, /test error/); resolve(); }), common.mustNotCall() @@ -46,7 +46,7 @@ function fail(binding) { }); } -function signalTest(binding) { +function signalTest (binding) { return new Promise((resolve, reject) => { const expectedCalls = 4; let actualCalls = 0; From 7c7bbfc156015ff2c3568edd56dd5c37b517bbec Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 27 Oct 2021 11:25:12 +1100 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: legendecas --- test/async_progress_worker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/async_progress_worker.js b/test/async_progress_worker.js index 529012528..776647045 100644 --- a/test/async_progress_worker.js +++ b/test/async_progress_worker.js @@ -65,7 +65,7 @@ function signalTest (binding) { return new Promise((resolve, reject) => { const expectedCalls = 3; let actualCalls = 0; - binding.doWork(expectedCalls, + binding.doSignalTest(expectedCalls, common.mustCall((err) => { if (err) { reject(err); From 928ed8405ed7285f2b0ef7c5a47333ab58637dab Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 27 Oct 2021 18:14:44 +1100 Subject: [PATCH 8/9] Add const back to ExecutionProgress --- napi-inl.h | 8 ++++---- napi.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index a816e3bdd..81c081cfa 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -5877,8 +5877,8 @@ inline void AsyncProgressWorker::Signal() { this->NonBlockingCall(static_cast(nullptr)); } -template -inline void AsyncProgressWorker::ExecutionProgress::Signal() { +template +inline void AsyncProgressWorker::ExecutionProgress::Signal() const { _worker->Signal(); } @@ -5995,8 +5995,8 @@ inline void AsyncProgressQueueWorker::OnWorkComplete(Napi::Env env, napi_stat AsyncProgressWorkerBase>::OnWorkComplete(env, status); } -template -inline void AsyncProgressQueueWorker::ExecutionProgress::Signal() { +template +inline void AsyncProgressQueueWorker::ExecutionProgress::Signal() const { _worker->Signal(); } diff --git a/napi.h b/napi.h index aed2769cc..a3619c3d9 100644 --- a/napi.h +++ b/napi.h @@ -2835,7 +2835,7 @@ namespace Napi { class ExecutionProgress { friend class AsyncProgressWorker; public: - void Signal(); + void Signal() const; void Send(const T* data, size_t count) const; private: explicit ExecutionProgress(AsyncProgressWorker* worker) : _worker(worker) {} @@ -2892,7 +2892,7 @@ namespace Napi { class ExecutionProgress { friend class AsyncProgressQueueWorker; public: - void Signal(); + void Signal() const; void Send(const T* data, size_t count) const; private: explicit ExecutionProgress(AsyncProgressQueueWorker* worker) : _worker(worker) {} From 8f0b85b03ee9c543ac9f5639cdc5140d3a825c9b Mon Sep 17 00:00:00 2001 From: Josephus Paye II Date: Wed, 27 Oct 2021 18:23:23 +1100 Subject: [PATCH 9/9] Add back const to Async*Worker::Execute() --- napi.h | 4 ++-- test/async_progress_queue_worker.cc | 4 ++-- test/async_progress_worker.cc | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/napi.h b/napi.h index a3619c3d9..e73a11bd1 100644 --- a/napi.h +++ b/napi.h @@ -2871,7 +2871,7 @@ namespace Napi { const char* resource_name, const Object& resource); #endif - virtual void Execute(ExecutionProgress& progress) = 0; + virtual void Execute(const ExecutionProgress& progress) = 0; virtual void OnProgress(const T* data, size_t count) = 0; private: @@ -2929,7 +2929,7 @@ namespace Napi { const char* resource_name, const Object& resource); #endif - virtual void Execute(ExecutionProgress& progress) = 0; + virtual void Execute(const ExecutionProgress& progress) = 0; virtual void OnProgress(const T* data, size_t count) = 0; private: diff --git a/test/async_progress_queue_worker.cc b/test/async_progress_queue_worker.cc index 4faf6da98..0e4a4a874 100644 --- a/test/async_progress_queue_worker.cc +++ b/test/async_progress_queue_worker.cc @@ -35,7 +35,7 @@ class TestWorker : public AsyncProgressQueueWorker { } protected: - void Execute(ExecutionProgress& progress) override { + void Execute(const ExecutionProgress& progress) override { using namespace std::chrono_literals; std::this_thread::sleep_for(1s); @@ -91,7 +91,7 @@ class SignalTestWorker : public AsyncProgressQueueWorker { } protected: - void Execute(ExecutionProgress& progress) override { + void Execute(const ExecutionProgress& progress) override { using namespace std::chrono_literals; std::this_thread::sleep_for(1s); diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index a2a2e2185..a2140a148 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -29,7 +29,7 @@ class TestWorker : public AsyncProgressWorker { } protected: - void Execute(ExecutionProgress& progress) override { + void Execute(const ExecutionProgress& progress) override { if (_times < 0) { SetError("test error"); } @@ -77,7 +77,7 @@ class MalignWorker : public AsyncProgressWorker { } protected: - void Execute(ExecutionProgress& progress) override { + void Execute(const ExecutionProgress& progress) override { std::unique_lock lock(_cvm); // Testing a nullptr send is acceptable. progress.Send(nullptr, 0); @@ -137,7 +137,7 @@ class SignalTestWorker : public AsyncProgressWorker { } protected: - void Execute(ExecutionProgress& progress) override { + void Execute(const ExecutionProgress& progress) override { if (_times < 0) { SetError("test error"); }