Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: empty data OnProgress by race conditions of AsyncProgressWorker #853

Closed
wants to merge 5 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 11, 2020

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.

@mhdawson mhdawson mentioned this pull request Dec 11, 2020
9 tasks
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

mhdawson commented Dec 11, 2020

@legendecas I tried to land this and had 2 issues

  1. it reported an unused value warning on line 90 of test/asyncprogressworker.cc. I update the line so not an issue:
-  void OnProgress(const ProgressData* data, size_t count) override {
+  void OnProgress(const ProgressData*, size_t count) override {
  1. The tests seem to stop after the asyncprogressworker test:
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.

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendecas
Copy link
Member Author

@mhdawson The tests seem to stop after the asyncprogressworker test

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.

@legendecas
Copy link
Member Author

legendecas commented Dec 14, 2020

@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.

@legendecas
Copy link
Member Author

@mhdawson The tests seem to stop after the asyncprogressworker test

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.

Just filed WIP nodejs/node#36510. Need to figure out if there are any other cases that also swallowing exceptions.

@mhdawson
Copy link
Member

@legendecas thanks for figuring out the issue and fixing up the tests. They all pass for me, landing.

mhdawson pushed a commit that referenced this pull request Dec 14, 2020
Fixes: #821
PR-URL: #853
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed as 86feeeb

@mhdawson mhdawson closed this Dec 14, 2020
@legendecas legendecas deleted the async-progress-worker branch December 15, 2020 01:51
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsyncProgressWorker::OnProgress is invoked without arg sometimes
4 participants