fix: empty data OnProgress by race conditions of AsyncProgressWorker - #853
fix: empty data OnProgress by race conditions of AsyncProgressWorker#853legendecas wants to merge 5 commits into
Conversation
Sorry, something went wrong.
mhdawson
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
@legendecas I tried to land this and had 2 issues
- void OnProgress(const ProgressData* data, size_t count) override {
+ void OnProgress(const ProgressData*, size_t count) override {
napiVersion:6
napiVersion:6
Testing with N-API Version '6'.
Starting test suite
Running test 'addon_build'
>Preparing native addons to build
>Building addon: 'echo addon'
>Running test for: 'echo addon'
>Building addon: 'echo-addon'
>Running test for: 'echo-addon'
Running test 'addon'
Running test 'addon_data'
Running test 'arraybuffer'
Running test 'asynccontext'
Running test 'asyncprogressqueueworker'
Running test 'asyncprogressworker'We'll need to figure that out before this can land. |
Sorry, something went wrong.
NickNaso
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Yeah, I've identified the problem. There might be an exception leaking in threadsafe function callbacks in node core so that the promise chains of the node-addon-api are broke up yet the thrown JavaScript exception is not handled properly and the process exits silently. I'll try to file a minimum reproduce to get that fixed in node core. Besides that, I'll update the PR to get tests running. |
Sorry, something went wrong.
|
@mhdawson GitHub Actions are up and all tests get passed :) PTAL Seems like there are random hang up in the tests. I'll take a deep look into that. |
Sorry, something went wrong.
Just filed WIP nodejs/node#36510. Need to figure out if there are any other cases that also swallowing exceptions. |
Sorry, something went wrong.
|
@legendecas thanks for figuring out the issue and fixing up the tests. They all pass for me, landing. |
Sorry, something went wrong.
Fixes: nodejs/node-addon-api#821 PR-URL: nodejs/node-addon-api#853 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Fixes: nodejs/node-addon-api#821 PR-URL: nodejs/node-addon-api#853 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Fixes: nodejs/node-addon-api#821 PR-URL: nodejs/node-addon-api#853 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Fixes: nodejs/node-addon-api#821 PR-URL: nodejs/node-addon-api#853 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Fixes #821
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.
However, when the race condition happens, the AsyncProgressWorker::OnProgress is likely not been called yet. There is hardly a fix on ensuring the race condition not happening.
In summary, the fix is suppressing the unexpected duplicated calls of AsyncProgressWorker::OnProgress.