Skip to content

Implement AsyncProgressQueueWorker#585

Closed
legendecas wants to merge 8 commits intonodejs:masterfrom
legendecas:async-progress-queue-worker
Closed

Implement AsyncProgressQueueWorker#585
legendecas wants to merge 8 commits intonodejs:masterfrom
legendecas:async-progress-queue-worker

Conversation

@legendecas
Copy link
Copy Markdown
Member

@legendecas legendecas commented Nov 2, 2019 •
edited
Loading

Although this patch comes along with a lot of changes on either Napi::AsyncWorker and Napi::AsyncProgressWorker, there are no changes in the existing code expected. (i.e. tests of Napi::AsyncWorker and Napi::AsyncProgressWorker shall pass without any modification)

Fixes: #582

@legendecas legendecas marked this pull request as ready for review November 2, 2019 02:51
@legendecas legendecas force-pushed the async-progress-queue-worker branch 3 times, most recently from 5908617 to dc63ed8 Compare November 2, 2019 05:34
@legendecas
Copy link
Copy Markdown
Member Author

legendecas commented Nov 6, 2019

Shall hold until we landed #589.

@mhdawson mhdawson mentioned this pull request Nov 18, 2019
Closed
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jan 13, 2020

@legendecas #589 has now landed so you can rebase/proceed.

@legendecas legendecas force-pushed the async-progress-queue-worker branch from dc63ed8 to 01d157c Compare January 26, 2020 16:43
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jan 27, 2020

Discussed in last N-API team meeting, still one aspect to be completed, but @legendecas would like feedback on what is there so far.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Feb 17, 2020

@nodejs/addon-api this is ready for review.

gabrielschulhof
gabrielschulhof reviewed Feb 24, 2020
View reviewed changes
Comment thread doc/async_worker_variants.md Outdated
Comment thread doc/async_worker_variants.md Outdated
Comment thread doc/async_worker_variants.md Outdated
Comment thread doc/async_worker_variants.md Outdated
Comment thread doc/async_worker_variants.md Outdated
Comment thread test/asyncprogressqueueworker.cc Outdated
Comment thread test/asyncprogressqueueworker.cc Outdated
Comment thread test/asyncprogressqueueworker.cc
ProgressData data{0};
for (int32_t idx = 0; idx < _times; idx++) {
data.progress = idx;
progress.Send(&data, 1);
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Feb 24, 2020

Choose a reason for hiding this comment

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

I think we should copy here and free in the JS thread.

Comment thread test/asyncprogressqueueworker.cc Outdated
Comment thread test/asyncprogressqueueworker.cc Outdated
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Feb 27, 2020

I'm not sure putting the doc for both classes into the same file make sense. I think it would stick to having one file to doc each class as we have for most classes. @NickNaso what do you think?

@legendecas
Copy link
Copy Markdown
Member Author

legendecas commented Feb 28, 2020

@mhdawson: I'm not sure putting the doc for both classes into the same file make sense. I think it would stick to having one file to doc each class as we have for most classes. @NickNaso what do you think?

I put them in a single doc file because they shared much similar behavior. It can be verbose if we separate those docs into different files.

mhdawson
mhdawson approved these changes Mar 18, 2020
View reviewed changes
Copy link
Copy Markdown
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 pushed a commit that referenced this pull request Mar 18, 2020
@legendecas @mhdawson
src: implement AsyncProgressQueueWorker
ab01844 
PR-URL: #585
Fixes: #582
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Mar 18, 2020

Landed as ab01844

@mhdawson mhdawson closed this Mar 18, 2020
@legendecas legendecas deleted the async-progress-queue-worker branch March 19, 2020 02:38
@gabrielschulhof
Copy link
Copy Markdown
Contributor

gabrielschulhof commented Apr 6, 2020

According to git bisect, this causes #695. @legendecas, could you PTAL?

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: implement AsyncProgressQueueWorker
84af85d 
PR-URL: nodejs/node-addon-api#585
Fixes: nodejs/node-addon-api#582
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: implement AsyncProgressQueueWorker
0258f84 
PR-URL: nodejs/node-addon-api#585
Fixes: nodejs/node-addon-api#582
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: implement AsyncProgressQueueWorker
6da4aed 
PR-URL: nodejs/node-addon-api#585
Fixes: nodejs/node-addon-api#582
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: implement AsyncProgressQueueWorker
4372037 
PR-URL: nodejs/node-addon-api#585
Fixes: nodejs/node-addon-api#582
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow overriding AsyncWorker::OnWorkComplete etc

3 participants

@legendecas @mhdawson @gabrielschulhof