Implement AsyncProgressWorker #529

legendecas · 2019-08-27T13:17:04Z

Related: #473

npm test passed
test included
documents are added or changed

napi.h

test/asyncprogressworker.cc

mhdawson · 2019-08-27T15:41:19Z

CI run to check across platforms: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/503/nodes=rhel72-s390x/console

mhdawson · 2019-08-27T15:48:38Z

Seems to be a failure due to unused paramter in OnProgress. Will push a change to address that so can test further.

mhdawson · 2019-08-27T15:48:46Z

New CI run: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/504/

mhdawson · 2019-08-27T15:50:19Z

@legendecas can you take a look, seems to be failing in CI runs across platforms with:

All tests passed!
terminate called after throwing an instance of 'Napi::Error'
  what():  The expression evaluated to a falsy value:

  assert.ok(e instanceof Error)

Tests aborted with SIGABRT
npm ERR! Test failed.  See above for more details.

mhdawson · 2019-08-27T15:52:35Z

@legendecas sorry, see you are still working on the tests from your checklist in the original post. Let us know when it's ready to review.

One other thing that you'll need to add is docs in the "Async Operations" section of https://github.com/nodejs/node-addon-api

legendecas · 2019-08-27T15:54:00Z

@legendecas can you take a look, seems to be failing in CI runs across platforms with:

All tests passed!
terminate called after throwing an instance of 'Napi::Error'
  what():  The expression evaluated to a falsy value:

  assert.ok(e instanceof Error)

Tests aborted with SIGABRT
npm ERR! Test failed.  See above for more details.

Yes, it's crashed for case https://github.com/nodejs/node-addon-api/blob/master/test/asyncworker.js. It could be reproduced locally.

legendecas · 2019-08-27T15:54:47Z

One other thing that you'll need to add is docs in the "Async Operations" section of https://github.com/nodejs/node-addon-api

Added to checklist

napi-inl.h

napi.h

test/asyncprogressworker.js

NickNaso

@legendecas I downloaded your work and it seems to work on macOS, but to execute the tests you need to add asyncprogressworker to here https://github.com/legendecas/node-addon-api/blob/async-progress-work/test/index.js#L13 and remove it in case the N-API version is less then 4 like we did here https://github.com/legendecas/node-addon-api/blob/async-progress-work/test/index.js#L67

legendecas · 2019-09-03T01:47:10Z

#532 would blocking this PR from their routine work like CI/tests. (Just a note)

mhdawson · 2019-09-27T21:57:33Z

Was a bit worried that some of the locking features might have requuired later compilers. This CI run seems to confirm it can build/run on Node.js 8.X so that is not an issue: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/675/

mhdawson · 2019-09-27T21:58:42Z

@legendecas sorry it's taking so long to get the review done. I'm out of time for today but I'll try to continue reviewing next week.

napi.h

napi-inl.h

mhdawson

LGTM, @legendecas thanks for all of the work on this. Hope to do a pass of landing PRs in the next few days.

gabrielschulhof

Partial review. Will continue.

doc/async_progress_worker.md

gabrielschulhof · 2019-10-14T15:41:24Z

doc/async_progress_worker.md

+
+    void OnProgress(const uint32_t* data, size_t /* count */) {
+        HandleScope scope(Env());
+        Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), data)});


data here is a pointer to an integer, not an integer. Also, there is no synchronization between the Execute writing to i and OnProgress reading from i. Perhaps above the code should be changed to

progress.Send(new uint32_t(i), 1);

and here it should be

Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), *data)}); delete data;

Yes, it's a pointer to an integer. I'll update the doc.

For the later question, as documented above, progress.Send will copy the data pointed to right on the call to the progress.Send. So it is safe to do so without any synchronizations.

doc/async_progress_worker.md

napi-inl.h

gabrielschulhof · 2019-10-29T20:03:18Z

doc/async_progress_worker.md

@@ -0,0 +1,344 @@
+# AsyncProgressWorker
+
+`Napi::AsyncProgressWorker` is an abstract class, which implements `Napi::AsyncWorker`


Suggested change

`Napi::AsyncProgressWorker` is an abstract class, which implements `Napi::AsyncWorker`

`Napi::AsyncProgressWorker` is an abstract class which implements `Napi::AsyncWorker`

gabrielschulhof · 2019-10-29T20:05:24Z

doc/async_progress_worker.md

+
+## Methods
+
+[`Napi::AsyncWorker`]() provides detailed descriptions for most methods.


Suggested change

[`Napi::AsyncWorker`]() provides detailed descriptions for most methods.

[`Napi::AsyncWorker`][] provides detailed descriptions for most methods.

gabrielschulhof · 2019-10-29T20:29:42Z

doc/async_progress_worker.md

+explicit Napi::AsyncProgressWorker(const Napi::Object& receiver, const Napi::Function& callback, const char* resource_name, const Napi::Object& resource);
+```
+
+- `[in] receiver`: The `this` object passed to the called function.


Suggested change

- `[in] receiver`: The `this` object passed to the called function.

- `[in] receiver`: The `this` object to pass to the called function.

doc/async_progress_worker.md

gabrielschulhof · 2019-10-29T20:46:16Z

doc/async_progress_worker.md

+passed in through its constructor.
+
+During the worker thread execution, the first argument of `Napi::AsyncProgressWorker::Execute`
+can be used to report the process of the execution.


Suggested change

can be used to report the process of the execution.

can be used to report the progress of the execution.

gabrielschulhof · 2019-10-29T20:50:18Z

doc/async_progress_worker.md

+callback that the `Napi::AsyncProgressWorker` base class will store persistently. When
+the work on the `Napi::AsyncProgressWorker::Execute` method is done the
+`Napi::AsyncProgressWorker::OnOk` method is called and the results return back to
+JavaScript invoking the stored callback with its associated environment.


Suggested change

JavaScript invoking the stored callback with its associated environment.

JavaScript when the stored callback is invoked with its associated environment.

gabrielschulhof · 2019-10-29T20:50:39Z

doc/async_progress_worker.md

+The following code shows an example of how to create and use an `Napi::AsyncProgressWorker`
+
+```cpp
+#include<napi.h>


Suggested change

#include<napi.h>

#include <napi.h>

gabrielschulhof · 2019-10-29T20:58:42Z

doc/async_progress_worker.md

+Using the implementation of a `Napi::AsyncProgressWorker` is straight forward. You only
+need to create a new instance and pass to its constructor the callback you want to
+execute when your asynchronous task ends and other data you need for your
+computation. Once created the only other action you have to do is to call the
+`Napi::AsyncProgressWorker::Queue` method that will queue the created worker for execution.


We generally avoid using "you" in our documentation. Let's rephrase this as

The implementation of a `Napi::AsyncProgressWorker` can be used by creating a new instance and passing to its constructore the callback to execute when the asynchronous task ends and other data needed for the computation. Once created, the only other action needed is to call the `Napi::AsyncProgressWorker::Queue` method that will queue the created worker for execution.

gabrielschulhof · 2019-10-29T20:59:05Z

doc/async_progress_worker.md

+
+The first step to use the `Napi::AsyncProgressWorker` class is to create a new class that
+inherits from it and implement the `Napi::AsyncProgressWorker::Execute` abstract method.
+Typically input to your worker will be saved within the class' fields generally


Suggested change

Typically input to your worker will be saved within the class' fields generally

Typically input to the worker will be saved within the class' fields generally

gabrielschulhof · 2019-10-29T20:59:22Z

doc/async_progress_worker.md

+use namespace Napi;
+
+Value Echo(const CallbackInfo& info) {
+    // You need to validate the arguments here


Suggested change

// You need to validate the arguments here

// We need to validate the arguments here

PR-URL: #529 Fixes: #473 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>

mhdawson · 2019-11-01T20:29:48Z

Landed as 650562c

PR-URL: nodejs/node-addon-api#529 Fixes: nodejs/node-addon-api#473 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>

mhdawson reviewed Aug 27, 2019

View reviewed changes

napi.h Outdated Show resolved Hide resolved

mhdawson reviewed Aug 27, 2019

View reviewed changes

test/asyncprogressworker.cc Outdated Show resolved Hide resolved

NickNaso reviewed Aug 27, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

napi-inl.h Outdated Show resolved Hide resolved

napi.h Outdated Show resolved Hide resolved

legendecas force-pushed the async-progress-work branch 2 times, most recently from f7d9fd4 to a9093d9 Compare August 28, 2019 09:22

mhdawson reviewed Aug 28, 2019

View reviewed changes

test/asyncprogressworker.js Outdated Show resolved Hide resolved

NickNaso reviewed Aug 28, 2019

View reviewed changes

legendecas force-pushed the async-progress-work branch from a9093d9 to 16f7249 Compare September 16, 2019 09:35

legendecas marked this pull request as ready for review September 16, 2019 09:35

legendecas force-pushed the async-progress-work branch from 16f7249 to 3c3d321 Compare September 16, 2019 14:32

mhdawson mentioned this pull request Sep 16, 2019

Do we need some additional AsyncWorker variants nodejs/abi-stable-node#373

Closed

legendecas force-pushed the async-progress-work branch from 3c3d321 to f150a98 Compare September 17, 2019 03:58

mhdawson reviewed Oct 3, 2019

View reviewed changes

napi.h Outdated Show resolved Hide resolved

mhdawson reviewed Oct 3, 2019

View reviewed changes

napi.h Show resolved Hide resolved

mhdawson reviewed Oct 3, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

legendecas mentioned this pull request Oct 4, 2019

Remove unnecessary local variable declaration nodejs/nan#870

Merged

1 task

legendecas force-pushed the async-progress-work branch 3 times, most recently from 521711b to 2540375 Compare October 6, 2019 14:54

mhdawson reviewed Oct 9, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

mhdawson approved these changes Oct 10, 2019

View reviewed changes

gabrielschulhof reviewed Oct 11, 2019

View reviewed changes

gabrielschulhof reviewed Oct 14, 2019

View reviewed changes

NickNaso mentioned this pull request Oct 22, 2019

Next release 2.0.0 #570

Closed

15 tasks

legendecas force-pushed the async-progress-work branch 2 times, most recently from 7e0e636 to b9748f2 Compare October 23, 2019 12:22

legendecas mentioned this pull request Oct 23, 2019

doc: improve AsyncWorker docs #571

Merged

legendecas force-pushed the async-progress-work branch from 21fd0ba to 5c11a56 Compare October 23, 2019 15:19

Implement AsyncProgressWorker

390507c

legendecas force-pushed the async-progress-work branch from 5c11a56 to 390507c Compare October 23, 2019 15:22

gabrielschulhof approved these changes Oct 29, 2019

View reviewed changes

doc: improve async progress worker doc

94f29d6

mhdawson pushed a commit that referenced this pull request Nov 1, 2019

src: implement AsyncProgressWorker

650562c

PR-URL: #529 Fixes: #473 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>

mhdawson closed this Nov 1, 2019

legendecas deleted the async-progress-work branch November 2, 2019 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AsyncProgressWorker #529

Implement AsyncProgressWorker #529

legendecas commented Aug 27, 2019 •

edited

Loading

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

legendecas commented Aug 27, 2019

legendecas commented Aug 27, 2019

NickNaso left a comment

legendecas commented Sep 3, 2019

mhdawson commented Sep 27, 2019 •

edited

Loading

mhdawson commented Sep 27, 2019

mhdawson left a comment

gabrielschulhof left a comment

gabrielschulhof Oct 14, 2019

legendecas Oct 23, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

gabrielschulhof Oct 29, 2019

mhdawson commented Nov 1, 2019

		@@ -0,0 +1,344 @@
		# AsyncProgressWorker

		`Napi::AsyncProgressWorker` is an abstract class, which implements `Napi::AsyncWorker`


		## Methods

		[`Napi::AsyncWorker`]() provides detailed descriptions for most methods.

	[`Napi::AsyncWorker`]() provides detailed descriptions for most methods.
	[`Napi::AsyncWorker`][] provides detailed descriptions for most methods.

	- `[in] receiver`: The `this` object passed to the called function.
	- `[in] receiver`: The `this` object to pass to the called function.

	can be used to report the process of the execution.
	can be used to report the progress of the execution.

	JavaScript invoking the stored callback with its associated environment.
	JavaScript when the stored callback is invoked with its associated environment.

	Typically input to your worker will be saved within the class' fields generally
	Typically input to the worker will be saved within the class' fields generally

	// You need to validate the arguments here
	// We need to validate the arguments here

Implement AsyncProgressWorker #529

Implement AsyncProgressWorker #529

Conversation

legendecas commented Aug 27, 2019 • edited Loading

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

mhdawson commented Aug 27, 2019

legendecas commented Aug 27, 2019

legendecas commented Aug 27, 2019

NickNaso left a comment

Choose a reason for hiding this comment

legendecas commented Sep 3, 2019

mhdawson commented Sep 27, 2019 • edited Loading

mhdawson commented Sep 27, 2019

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 1, 2019

legendecas commented Aug 27, 2019 •

edited

Loading

mhdawson commented Sep 27, 2019 •

edited

Loading