src: fix implementation of AsyncProgressWorker::Signal #1216
Conversation
test/async_progress_worker.cc
Outdated
| { | ||
| std::unique_lock<std::mutex> lock(_cvm); | ||
| // Testing a nullptr send is acceptable. | ||
| progress.Send(nullptr, 0); | ||
| _cv.wait(lock); | ||
| } | ||
| { | ||
| std::unique_lock<std::mutex> lock(_cvm); | ||
| progress.Signal(); | ||
| _cv.wait(lock); | ||
| } |
There was a problem hiding this comment.
Use the cv wait overload that has a condition function
|
@KevinEady could you add a short explanation of how using SendProgress instead of Signal resolves the problem? I think I know but not 100% sure. @legendecas would be good if you reviewed as well since it seems like you were in the loop on the discussions of the original problem. |
|
Hi @mhdawson , The I went about this PR by looking at the existing test we have: node-addon-api/test/async_progress_worker.cc Lines 82 to 83 in a8afb2d Since sending Hope this clarifies! Thanks, Kevin |
|
@KevinEady many thanks for the explanation |
There was a problem hiding this comment.
Sorry for the delay.
AsyncProgressWorker::Signal should not invalidate sent progress not handled, similar to https://github.com/nodejs/nan/blob/main/nan.h#L2088. Would you mind adding a test case for this?
napi-inl.h
Outdated
| @@ -6032,12 +6032,12 @@ inline void AsyncProgressWorker<T>::SendProgress_(const T* data, size_t count) { | |||
|
|
|||
| template <class T> | |||
| inline void AsyncProgressWorker<T>::Signal() const { | |||
| this->NonBlockingCall(static_cast<T*>(nullptr)); | |||
| this->SendProgress_(static_cast<T*>(nullptr), 0); | |||
There was a problem hiding this comment.
This will override saved progress and make unexpected side effects. We should save a bit of the signal and proceed with null data at https://github.com/nodejs/node-addon-api/blob/main/napi-inl.h#L5998 instead.
There was a problem hiding this comment.
Hi @legendecas , please take a look at 1f2cd71. Is this what you are talking about with 'save a bit of the signal'?
I also added a test case were an OnProgress and Signal are performed in sequence, and checks that the progress data is present.
|
Another thing @legendecas , i did not make any changes to |
There was a problem hiding this comment.
LGTM. I think it would be great to add a caveat on https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker_variants.md#onprogress about the nullability of the data parameter if the method Signal is used.
PR-URL: #1216 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
|
Landed as edf630c |
PR-URL: nodejs/node-addon-api#1216 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Fix implementation of
AsyncProgressWorker::SignalandAsyncProgressQueueWorker::SignalFixes: #1095
Fixes: #1081