From 3520789c2151df97d41ac4354040053f793c6471 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 11 Dec 2020 19:44:59 +0800 Subject: [PATCH 1/5] fix: empty data OnProgress by race conditions of AsyncProgressWorker --- napi-inl.h | 11 +++++++ test/asyncprogressworker.cc | 59 +++++++++++++++++++++++++++++++++++++ test/asyncprogressworker.js | 15 ++++++++++ test/common/index.js | 3 ++ 4 files changed, 88 insertions(+) diff --git a/napi-inl.h b/napi-inl.h index 5a345df53..4e8e8ef11 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -5390,6 +5390,17 @@ inline void AsyncProgressWorker::OnWorkProgress(void*) { this->_asyncsize = 0; } + /** + * The callback of ThreadSafeFunction is not been invoked immediately on the + * callback of uv_async_t (uv io poll), rather the callback of TSFN is + * invoked on the right next uv idle callback. There are chances that during + * 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) { + return; + } + this->OnProgress(data, size); delete[] data; } diff --git a/test/asyncprogressworker.cc b/test/asyncprogressworker.cc index 8ac062fcb..c2f27e182 100644 --- a/test/asyncprogressworker.cc +++ b/test/asyncprogressworker.cc @@ -61,11 +61,70 @@ class TestWorker : public AsyncProgressWorker { FunctionReference _progress; }; +class MalignWorker : public AsyncProgressWorker { + public: + static void DoWork(const CallbackInfo& info) { + Function cb = info[0].As(); + Function progress = info[1].As(); + + MalignWorker* worker = + new MalignWorker(cb, progress, "TestResource", Object::New(info.Env())); + worker->Queue(); + } + + 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); + // Testing busy looping on send doesn't trigger unexpected empty data + // OnProgress call. + for (size_t i = 0; i < 1000000; i++) { + ProgressData data{0}; + progress.Send(&data, 1); + } + _cv.wait(lock); + } + + void OnProgress(const ProgressData* data, size_t count) override { + Napi::Env env = Env(); + _test_case_count++; + bool error = false; + Napi::String reason = Napi::String::New(env, "No error"); + if (_test_case_count == 1 && count != 0) { + error = true; + reason = Napi::String::New(env, "expect 0 count of data on 1st call"); + } + if (_test_case_count > 1 && count != 1) { + error = true; + reason = Napi::String::New(env, "expect 1 count of data on non-1st call"); + } + _progress.MakeCallback(Receiver().Value(), + {Napi::Boolean::New(env, error), reason}); + _cv.notify_one(); + } + + private: + MalignWorker(Function cb, + Function progress, + const char* resource_name, + const Object& resource) + : AsyncProgressWorker(cb, resource_name, resource) { + _progress.Reset(progress, 1); + } + + size_t _test_case_count = 0; + std::condition_variable _cv; + std::mutex _cvm; + FunctionReference _progress; +}; } Object InitAsyncProgressWorker(Env env) { Object exports = Object::New(env); exports["doWork"] = Function::New(env, TestWorker::DoWork); + exports["doMalignTest"] = Function::New(env, MalignWorker::DoWork); return exports; } diff --git a/test/asyncprogressworker.js b/test/asyncprogressworker.js index b2896a6ca..bbb43fc90 100644 --- a/test/asyncprogressworker.js +++ b/test/asyncprogressworker.js @@ -9,6 +9,7 @@ module.exports = test(require(`./build/${buildType}/binding.node`)) async function test({ asyncprogressworker }) { await success(asyncprogressworker); await fail(asyncprogressworker); + await malignTest(asyncprogressworker); } function success(binding) { @@ -43,3 +44,17 @@ function fail(binding) { ); }); } + +function malignTest(binding) { + return new Promise((resolve, reject) => { + binding.doMalignTest( + common.mustCall((err) => { + assert.throws(() => { throw err }, /test error/); + resolve(); + }), + common.mustCallAtLeast((error, reason) => { + assert(!error, reason); + }, 1) + ); + }); +} diff --git a/test/common/index.js b/test/common/index.js index 57bc525ef..54139bb2d 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -33,6 +33,9 @@ function runCallChecks(exitCode) { exports.mustCall = function(fn, exact) { return _mustCallInner(fn, exact, 'exact'); }; +exports.mustCallAtLeast = function(fn, minimum) { + return _mustCallInner(fn, minimum, 'minimum'); +}; function _mustCallInner(fn, criteria, field) { if (typeof fn === 'number') { From e7cdc8ecac4932363d51e1cf00ffb8899e443b0c Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 14 Dec 2020 12:04:11 +0800 Subject: [PATCH 2/5] fixup! fix: empty data OnProgress by race conditions of AsyncProgressWorker --- test/asyncprogressworker.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/asyncprogressworker.js b/test/asyncprogressworker.js index bbb43fc90..285fd2a91 100644 --- a/test/asyncprogressworker.js +++ b/test/asyncprogressworker.js @@ -49,7 +49,9 @@ function malignTest(binding) { return new Promise((resolve, reject) => { binding.doMalignTest( common.mustCall((err) => { - assert.throws(() => { throw err }, /test error/); + if (err) { + return reject(err); + } resolve(); }), common.mustCallAtLeast((error, reason) => { From df0a6da390911bfa818214239c9b9d6f81f954d6 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 14 Dec 2020 12:07:32 +0800 Subject: [PATCH 3/5] fixup! fix: empty data OnProgress by race conditions of AsyncProgressWorker --- test/asyncprogressworker.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/asyncprogressworker.cc b/test/asyncprogressworker.cc index c2f27e182..679a43fa9 100644 --- a/test/asyncprogressworker.cc +++ b/test/asyncprogressworker.cc @@ -87,7 +87,7 @@ class MalignWorker : public AsyncProgressWorker { _cv.wait(lock); } - 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++; bool error = false; From a1253114ed695b74aa1cfdd8adbb8f5ac006ce90 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 15 Dec 2020 01:13:18 +0800 Subject: [PATCH 4/5] fixup! fix: empty data OnProgress by race conditions of AsyncProgressWorker --- test/asyncprogressworker.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/asyncprogressworker.cc b/test/asyncprogressworker.cc index 679a43fa9..1705124ad 100644 --- a/test/asyncprogressworker.cc +++ b/test/asyncprogressworker.cc @@ -84,7 +84,6 @@ class MalignWorker : public AsyncProgressWorker { ProgressData data{0}; progress.Send(&data, 1); } - _cv.wait(lock); } void OnProgress(const ProgressData* /* data */, size_t count) override { From e379aee31df1af1628b9e5f561d16b9d5e21873b Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 15 Dec 2020 01:14:51 +0800 Subject: [PATCH 5/5] script: add timeout options on tests --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e85a66c3f..193f09384 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,7 @@ on: [push, pull_request] jobs: test: + timeout-minutes: 30 strategy: matrix: node-version: