From 1c1bac126a2d415ac0f6eb12617e182915733b0c Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Fri, 7 Oct 2022 16:16:55 +0200 Subject: [PATCH 1/4] src: fix implementation of Signal --- napi-inl.h | 8 ++++---- test/async_progress_queue_worker.cc | 16 +++++++++++++--- test/async_progress_worker.cc | 25 +++++++++++++++++-------- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 2583f81f8..e5fa86883 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -6032,12 +6032,12 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { template inline void AsyncProgressWorker::Signal() const { - this->NonBlockingCall(static_cast(nullptr)); + this->SendProgress_(static_cast(nullptr), 0); } template inline void AsyncProgressWorker::ExecutionProgress::Signal() const { - _worker->Signal(); + _worker->SendProgress_(static_cast(nullptr), 0); } template @@ -6140,7 +6140,7 @@ inline void AsyncProgressQueueWorker::SendProgress_(const T* data, template inline void AsyncProgressQueueWorker::Signal() const { - this->NonBlockingCall(nullptr); + this->SendProgress_(static_cast(nullptr), 0); } template @@ -6152,7 +6152,7 @@ inline void AsyncProgressQueueWorker::OnWorkComplete(Napi::Env env, template inline void AsyncProgressQueueWorker::ExecutionProgress::Signal() const { - _worker->Signal(); + _worker->SendProgress_(static_cast(nullptr), 0); } template diff --git a/test/async_progress_queue_worker.cc b/test/async_progress_queue_worker.cc index eec3f9510..ace2e8fd1 100644 --- a/test/async_progress_queue_worker.cc +++ b/test/async_progress_queue_worker.cc @@ -41,6 +41,8 @@ class TestWorker : public AsyncProgressQueueWorker { if (_times < 0) { SetError("test error"); + } else { + progress.Signal(); } ProgressData data{0}; for (int32_t idx = 0; idx < _times; idx++) { @@ -49,11 +51,18 @@ class TestWorker : public AsyncProgressQueueWorker { } } - void OnProgress(const ProgressData* data, size_t /* count */) override { + void OnProgress(const ProgressData* data, size_t count) override { Napi::Env env = Env(); + _test_case_count++; if (!_js_progress_cb.IsEmpty()) { - Number progress = Number::New(env, data->progress); - _js_progress_cb.Call(Receiver().Value(), {progress}); + if (_test_case_count == 1) { + if (count != 0) { + SetError("expect 0 count of data on 1st call"); + } + } else { + Number progress = Number::New(env, data->progress); + _js_progress_cb.Call(Receiver().Value(), {progress}); + } } } @@ -68,6 +77,7 @@ class TestWorker : public AsyncProgressQueueWorker { } int32_t _times; + size_t _test_case_count = 0; FunctionReference _js_progress_cb; }; diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index 36087e7ca..ac6d3cf14 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -78,10 +78,17 @@ class MalignWorker : public AsyncProgressWorker { protected: void Execute(const ExecutionProgress& progress) override { - std::unique_lock lock(_cvm); - // Testing a nullptr send is acceptable. - progress.Send(nullptr, 0); - _cv.wait(lock); + { + std::unique_lock lock(_cvm); + // Testing a nullptr send is acceptable. + progress.Send(nullptr, 0); + _cv.wait(lock); + } + { + std::unique_lock lock(_cvm); + progress.Signal(); + _cv.wait(lock); + } // Testing busy looping on send doesn't trigger unexpected empty data // OnProgress call. for (size_t i = 0; i < 1000000; i++) { @@ -95,13 +102,15 @@ class MalignWorker : public AsyncProgressWorker { _test_case_count++; bool error = false; Napi::String reason = Napi::String::New(env, "No error"); - if (_test_case_count == 1 && count != 0) { + if (_test_case_count <= 2 && count != 0) { error = true; - reason = Napi::String::New(env, "expect 0 count of data on 1st call"); + reason = + Napi::String::New(env, "expect 0 count of data on 1st and 2nd call"); } - if (_test_case_count > 1 && count != 1) { + if (_test_case_count > 2 && count != 1) { error = true; - reason = Napi::String::New(env, "expect 1 count of data on non-1st call"); + reason = Napi::String::New( + env, "expect 1 count of data on non-1st and non-2nd call"); } _progress.MakeCallback(Receiver().Value(), {Napi::Boolean::New(env, error), reason}); From 45ffe918310ebb51c20a8ab66a137fa091195c49 Mon Sep 17 00:00:00 2001 From: Joe McCormick Date: Tue, 11 Oct 2022 05:45:12 -0400 Subject: [PATCH 2/4] impl lock_guard and lock wait return --- test/async_progress_worker.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index ac6d3cf14..ce2a37f3c 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -82,12 +82,12 @@ class MalignWorker : public AsyncProgressWorker { std::unique_lock lock(_cvm); // Testing a nullptr send is acceptable. progress.Send(nullptr, 0); - _cv.wait(lock); + _cv.wait(lock, [this] { return _test_case_count == 1; }); } { std::unique_lock lock(_cvm); progress.Signal(); - _cv.wait(lock); + _cv.wait(lock, [this] { return _test_case_count == 2; }); } // Testing busy looping on send doesn't trigger unexpected empty data // OnProgress call. @@ -99,7 +99,10 @@ class MalignWorker : public AsyncProgressWorker { void OnProgress(const ProgressData* /* data */, size_t count) override { Napi::Env env = Env(); - _test_case_count++; + { + std::lock_guard lock(_cvm); + _test_case_count++; + } bool error = false; Napi::String reason = Napi::String::New(env, "No error"); if (_test_case_count <= 2 && count != 0) { From 1f2cd71a281c393ae7663b3c9513030bf3fe1b5a Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Fri, 11 Nov 2022 16:25:19 +0100 Subject: [PATCH 3/4] change Signal() to use a state member --- napi-inl.h | 19 +++++++++++---- napi.h | 3 ++- test/async_progress_worker.cc | 44 +++++++++++++++++++++++++++++++++++ test/async_progress_worker.js | 13 +++++++---- 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index e5fa86883..fec439778 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -5950,7 +5950,8 @@ inline AsyncProgressWorker::AsyncProgressWorker(const Object& receiver, const Object& resource) : AsyncProgressWorkerBase(receiver, callback, resource_name, resource), _asyncdata(nullptr), - _asyncsize(0) {} + _asyncsize(0), + _signaled(false) {} #if NAPI_VERSION > 4 template @@ -5990,12 +5991,15 @@ template inline void AsyncProgressWorker::OnWorkProgress(void*) { T* data; size_t size; + bool signaled; { std::lock_guard lock(this->_mutex); data = this->_asyncdata; size = this->_asyncsize; + signaled = this->_signaled; this->_asyncdata = nullptr; this->_asyncsize = 0; + this->_signaled = false; } /** @@ -6005,7 +6009,7 @@ inline void AsyncProgressWorker::OnWorkProgress(void*) { * the deferring the signal of uv_async_t is been sent again, i.e. potential * not coalesced two calls of the TSFN callback. */ - if (data == nullptr) { + if (data == nullptr && !signaled) { return; } @@ -6024,6 +6028,7 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { old_data = _asyncdata; _asyncdata = new_data; _asyncsize = count; + _signaled = false; } this->NonBlockingCall(nullptr); @@ -6031,13 +6036,17 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { } template -inline void AsyncProgressWorker::Signal() const { - this->SendProgress_(static_cast(nullptr), 0); +inline void AsyncProgressWorker::Signal() { + { + std::lock_guard lock(this->_mutex); + _signaled = true; + } + this->NonBlockingCall(static_cast(nullptr)); } template inline void AsyncProgressWorker::ExecutionProgress::Signal() const { - _worker->SendProgress_(static_cast(nullptr), 0); + this->_worker->Signal(); } template diff --git a/napi.h b/napi.h index 0fd18c85f..9d0f78e1c 100644 --- a/napi.h +++ b/napi.h @@ -3007,12 +3007,13 @@ class AsyncProgressWorker : public AsyncProgressWorkerBase { private: void Execute() override; - void Signal() const; + void Signal(); void SendProgress_(const T* data, size_t count); std::mutex _mutex; T* _asyncdata; size_t _asyncsize; + bool _signaled; }; template diff --git a/test/async_progress_worker.cc b/test/async_progress_worker.cc index ce2a37f3c..3975bfbf4 100644 --- a/test/async_progress_worker.cc +++ b/test/async_progress_worker.cc @@ -134,12 +134,56 @@ class MalignWorker : public AsyncProgressWorker { std::mutex _cvm; FunctionReference _progress; }; + +// Calling a Signal after a SendProgress should not clear progress data +class SignalAfterProgressTestWorker : public AsyncProgressWorker { + public: + static void DoWork(const CallbackInfo& info) { + Function cb = info[0].As(); + Function progress = info[1].As(); + + SignalAfterProgressTestWorker* worker = new SignalAfterProgressTestWorker( + cb, progress, "TestResource", Object::New(info.Env())); + worker->Queue(); + } + + protected: + void Execute(const ExecutionProgress& progress) override { + ProgressData data{0}; + progress.Send(&data, 1); + progress.Signal(); + } + + void OnProgress(const ProgressData* /* data */, size_t count) override { + Napi::Env env = Env(); + bool error = false; + Napi::String reason = Napi::String::New(env, "No error"); + if (count != 1) { + error = true; + reason = Napi::String::New(env, "expect 1 count of data"); + } + _progress.MakeCallback(Receiver().Value(), + {Napi::Boolean::New(env, error), reason}); + } + + private: + SignalAfterProgressTestWorker(Function cb, + Function progress, + const char* resource_name, + const Object& resource) + : AsyncProgressWorker(cb, resource_name, resource) { + _progress.Reset(progress, 1); + } + 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["doSignalAfterProgressTest"] = + Function::New(env, SignalAfterProgressTestWorker::DoWork); return exports; } diff --git a/test/async_progress_worker.js b/test/async_progress_worker.js index 5e9940516..b7a1de367 100644 --- a/test/async_progress_worker.js +++ b/test/async_progress_worker.js @@ -8,7 +8,8 @@ module.exports = common.runTest(test); async function test ({ asyncprogressworker }) { await success(asyncprogressworker); await fail(asyncprogressworker); - await malignTest(asyncprogressworker); + await signalTest(asyncprogressworker.doMalignTest); + await signalTest(asyncprogressworker.doSignalAfterProgressTest); } function success (binding) { @@ -44,9 +45,9 @@ function fail (binding) { }); } -function malignTest (binding) { +function signalTest (bindingFunction) { return new Promise((resolve, reject) => { - binding.doMalignTest( + bindingFunction( common.mustCall((err) => { if (err) { return reject(err); @@ -54,7 +55,11 @@ function malignTest (binding) { resolve(); }), common.mustCallAtLeast((error, reason) => { - assert(!error, reason); + try { + assert(!error, reason); + } catch (e) { + reject(e); + } }, 1) ); }); From 6659e82b62724a6b9c606ab6ec8973ccf365ed4c Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Fri, 25 Nov 2022 16:03:35 +0100 Subject: [PATCH 4/4] update docs --- doc/async_worker_variants.md | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/doc/async_worker_variants.md b/doc/async_worker_variants.md index 591cd8a57..d2d32af0a 100644 --- a/doc/async_worker_variants.md +++ b/doc/async_worker_variants.md @@ -51,8 +51,11 @@ virtual void Napi::AsyncProgressWorker::OnOK(); ### OnProgress -This method is invoked when the computation in the `Napi::AsyncProgressWorker::ExecutionProcess::Send` -method was called during worker thread execution. +This method is invoked when the computation in the +`Napi::AsyncProgressWorker::ExecutionProcess::Send` method was called during +worker thread execution. This method can also be triggered via a call to +`Napi::AsyncProgress[Queue]Worker::ExecutionProcess::Signal`, in which case the +`data` parameter will be `nullptr`. ```cpp virtual void Napi::AsyncProgressWorker::OnProgress(const T* data, size_t count) @@ -251,6 +254,15 @@ class instead which is documented further down this page. void Napi::AsyncProgressWorker::ExecutionProcess::Send(const T* data, size_t count) const; ``` +### Signal + +`Napi::AsyncProgressWorker::ExecutionProcess::Signal` triggers an invocation of +`Napi::AsyncProgressWorker::OnProgress` with `nullptr` as the `data` parameter. + +```cpp +void Napi::AsyncProgressWorker::ExecutionProcess::Signal(); +``` + ## Example The first step to use the `Napi::AsyncProgressWorker` class is to create a new class that @@ -415,6 +427,15 @@ with each data item. void Napi::AsyncProgressQueueWorker::ExecutionProcess::Send(const T* data, size_t count) const; ``` +### Signal + +`Napi::AsyncProgressQueueWorker::ExecutionProcess::Signal` triggers an invocation of +`Napi::AsyncProgressQueueWorker::OnProgress` with `nullptr` as the `data` parameter. + +```cpp +void Napi::AsyncProgressQueueWorker::ExecutionProcess::Signal() const; +``` + ## Example The code below shows an example of the `Napi::AsyncProgressQueueWorker` implementation, but