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 diff --git a/napi-inl.h b/napi-inl.h index 3b79809c8..631045715 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -5940,7 +5940,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 @@ -5980,12 +5981,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; } /** @@ -5995,7 +5999,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; } @@ -6014,6 +6018,7 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { old_data = _asyncdata; _asyncdata = new_data; _asyncsize = count; + _signaled = false; } this->NonBlockingCall(nullptr); @@ -6021,13 +6026,17 @@ inline void AsyncProgressWorker::SendProgress_(const T* data, size_t count) { } template -inline void AsyncProgressWorker::Signal() const { +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->Signal(); + this->_worker->Signal(); } template @@ -6130,7 +6139,7 @@ inline void AsyncProgressQueueWorker::SendProgress_(const T* data, template inline void AsyncProgressQueueWorker::Signal() const { - this->NonBlockingCall(nullptr); + this->SendProgress_(static_cast(nullptr), 0); } template @@ -6142,7 +6151,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/napi.h b/napi.h index 58a0c523b..91208b107 100644 --- a/napi.h +++ b/napi.h @@ -3004,12 +3004,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_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..3975bfbf4 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, [this] { return _test_case_count == 1; }); + } + { + std::unique_lock lock(_cvm); + progress.Signal(); + _cv.wait(lock, [this] { return _test_case_count == 2; }); + } // Testing busy looping on send doesn't trigger unexpected empty data // OnProgress call. for (size_t i = 0; i < 1000000; i++) { @@ -92,16 +99,21 @@ 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 == 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}); @@ -122,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) ); });