src: make OnWorkComplete and OnExecute override-able by legendecas · Pull Request #589 · nodejs/node-addon-api

legendecas · 2019-11-06T12:03:30Z

No breaking changes on existing code were expected. All existing tests shall pass without any touch.
Changes on declaration:

Added Napi::AsyncWorker::OnWorkComplete.
Added Napi::AsyncWorker::OnExecute.

Fixes: #582

npm test passed.
documentation has changed.

legendecas · 2019-11-25T15:28:19Z

Napi::AsyncProgressWorkerBase should be able to accept an option to set the queue size of the tsfn and pass an opaque data pointer on its virtual OnWorkProgress method. Just wondering if this would produce another overhead on the TSFN callback since we already have an wrapper on the callback by Napi::ThreadSafeFunction.

Just mark the PR as working in progress.

legendecas · 2019-12-08T07:23:41Z

There might be issues on sharing the AsyncProgressWorker base class between AsyncProgressWorker and AsyncProgressQueueWorker if the ThreadSafeFunction queue size is different: a possible redundant wrapper around ThreadSafeFunction Call might be introduced to AsyncProgressWorker.

So I'd like to remove the AsyncProgressWorker base class from the PR and leave the PR for the AsyncWorker's methods.

mhdawson · 2019-12-09T16:10:11Z

This one is ready to be reviewed @nodejs/n-api please review/comment.

gabrielschulhof

LGTM % nits

gabrielschulhof · 2019-12-24T01:03:26Z


+### OnWorkComplete
+
+This method is invoked after the work has completed on JavaScript thread.


Nit:

Suggested change

This method is invoked after the work has completed on JavaScript thread.

This method is invoked after the work has completed on the JavaScript thread.

gabrielschulhof · 2019-12-25T03:34:33Z

+// The OnAsyncWorkExecute method receives an napi_env argument. However, do NOT
+// use it within this method, as it does not run on the main thread and must
+// not run any method that would cause JavaScript to run. In practice, this
+// means that almost any use of napi_env will be incorrect.


This blurb should be copied into the documentation above.

gabrielschulhof · 2019-12-25T03:35:35Z

 #ifdef NAPI_CPP_EXCEPTIONS
  try {
-    self->Execute();
+    this->Execute();


Do we need this-> here and in the lines below?

gabrielschulhof · 2019-12-25T03:35:57Z

+inline void AsyncWorker::OnWorkComplete(Napi::Env /*env*/, napi_status status) {
  if (status != napi_cancelled) {
-    HandleScope scope(self->_env);
+    HandleScope scope(this->_env);


Same here. I don't think we need this->.

Noticed some greater-than-nit things like the removal of this->

No breaking changes on existing code were expected. All existing tests shall pass without any touch. Changes on declaration: - Added `Napi::AsyncWorker::OnWorkComplete`. - Added `Napi::AsyncWorker::OnExecute`.

legendecas · 2020-01-01T13:18:35Z

@gabrielschulhof updated! PTAL :D

gabrielschulhof

LGTM with a few nits.

gabrielschulhof · 2020-01-01T18:40:55Z

+The default implementation of this method just calls the `Napi::AsyncWorker::Execute`
+and handles exceptions if cpp exceptions were enabled.
+
+The `OnExecute` method receives an `napi_env` argument. However, do NOT


Suggested change

The `OnExecute` method receives an `napi_env` argument. However, do NOT

The `OnExecute` method receives an `napi_env` argument. However, it must NOT be used

gabrielschulhof · 2020-01-01T18:41:13Z

+and handles exceptions if cpp exceptions were enabled.
+
+The `OnExecute` method receives an `napi_env` argument. However, do NOT
+use it within this method, as it does not run on the JavaScript thread and


Suggested change

use it within this method, as it does not run on the JavaScript thread and

within this method, as it does not run on the JavaScript thread and

gabrielschulhof · 2020-01-05T20:59:18Z

CI:

Version	Job	Status
v14.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1541/	✓
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1542/	✓
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1543/	✓

mhdawson

LGTM

gabrielschulhof · 2020-01-10T00:03:56Z

CI:

Version	Job	Status
v14.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1581/	✓
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1582/	✗
	https://ci.nodejs.org/job/node-test-node-addon-api-new/1584/	✗
	https://ci.nodejs.org/job/node-test-node-addon-api-new/1585/	✓
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1583/	✓

No breaking changes on existing code were expected. All existing tests shall pass without any touch. Changes on declaration: - Added `Napi::AsyncWorker::OnWorkComplete`. - Added `Napi::AsyncWorker::OnExecute`. PR-URL: #589 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gabrielschulhof · 2020-01-10T00:06:09Z

Landed in 23ff7f0.

No breaking changes on existing code were expected. All existing tests shall pass without any touch. Changes on declaration: - Added `Napi::AsyncWorker::OnWorkComplete`. - Added `Napi::AsyncWorker::OnExecute`. PR-URL: nodejs/node-addon-api#589 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

legendecas force-pushed the async-worker branch 2 times, most recently from 0eb4bba to e1e5e6f Compare November 6, 2019 12:09

legendecas mentioned this pull request Nov 6, 2019

Implement AsyncProgressQueueWorker #585

Closed

4 tasks

legendecas force-pushed the async-worker branch from e1e5e6f to 016d91d Compare December 8, 2019 07:19

gabrielschulhof reviewed Dec 9, 2019

View reviewed changes

Comment thread doc/async_worker.md Outdated

Comment thread doc/async_worker.md Outdated

Comment thread doc/async_worker.md Outdated

Comment thread doc/async_worker.md Outdated

Comment thread doc/async_worker.md Outdated

Comment thread doc/async_worker.md Outdated

Comment thread napi.h Outdated

gabrielschulhof previously approved these changes Dec 24, 2019

View reviewed changes

legendecas force-pushed the async-worker branch from ba820aa to 3da380b Compare December 25, 2019 02:36

gabrielschulhof reviewed Dec 25, 2019

View reviewed changes

Comment thread doc/async_worker.md

gabrielschulhof reviewed Dec 25, 2019

View reviewed changes

src: make OnWorkComplete and OnExecute override-able

0de964c

No breaking changes on existing code were expected. All existing tests shall pass without any touch. Changes on declaration: - Added `Napi::AsyncWorker::OnWorkComplete`. - Added `Napi::AsyncWorker::OnExecute`.

legendecas force-pushed the async-worker branch from 3da380b to 0de964c Compare January 1, 2020 13:15

fixup! src: make OnWorkComplete and OnExecute override-able

02c69bc

gabrielschulhof approved these changes Jan 1, 2020

View reviewed changes

fixup! src: make OnWorkComplete and OnExecute override-able

b117b43

gabrielschulhof approved these changes Jan 5, 2020

View reviewed changes

mhdawson approved these changes Jan 9, 2020

View reviewed changes

gabrielschulhof closed this Jan 10, 2020

legendecas deleted the async-worker branch January 10, 2020 14:28

legendecas mentioned this pull request Feb 10, 2020

Allow overriding AsyncWorker::OnWorkComplete etc #582

Closed


		### OnWorkComplete

		This method is invoked after the work has completed on JavaScript thread.

	The `OnExecute` method receives an `napi_env` argument. However, do NOT
	The `OnExecute` method receives an `napi_env` argument. However, it must NOT be used

	use it within this method, as it does not run on the JavaScript thread and
	within this method, as it does not run on the JavaScript thread and

Uh oh!

Conversation

legendecas commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legendecas commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legendecas commented Dec 8, 2019

Uh oh!

mhdawson commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gabrielschulhof Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

legendecas commented Jan 1, 2020

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Jan 1, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Jan 1, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jan 10, 2020

Uh oh!

gabrielschulhof commented Jan 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

legendecas commented Nov 6, 2019 •

edited

Loading

legendecas commented Nov 25, 2019 •

edited

Loading

gabrielschulhof Dec 24, 2019 •

edited

Loading

gabrielschulhof commented Jan 5, 2020 •

edited

Loading