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.

Sign up for GitHub

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

Jump to bottom

Implement AsyncProgressQueueWorker #585

Closed
wants to merge 8 commits into from
Closed

Implement AsyncProgressQueueWorker #585

wants to merge 8 commits into from

Conversation

legendecas
Copy link
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
Member Author

Shall hold until we landed #589.

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

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

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
Member

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

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

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.

test/asyncprogressqueueworker.cc Outdated Show resolved Hide resolved
test/asyncprogressqueueworker.cc Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member

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
Member Author

@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
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
@mhdawson
Copy link
Member

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
Contributor

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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

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