Implement AsyncContext class by romandev · Pull Request #252 · nodejs/node-addon-api

romandev · 2018-04-30T14:38:19Z

This class provides a wrapper for the following custom asynchronous
operation APIs.

napi_async_init()
napi_async_destroy()
napi_make_callback()

This PR is initiated from #140 (comment).

mhdawson · 2018-05-01T19:41:05Z

@romandev thanks for putting this together. Can you add the doc for this class? I have a few questions but I think the doc would likely answer them and we'll need it anyway.

mhdawson · 2018-07-09T21:31:23Z

@romandev are you going to have a chance to get back to this?

romandev · 2018-07-09T23:18:33Z

@mhdawson, I'll update this patch in this week.

romandev · 2018-07-11T08:23:48Z

@mhdawson I've pushed a new patch. Please take a look.

NickNaso

Great work and some notes about documentation. It could be good add one simple usage example at the end of documentation.

NickNaso · 2018-07-11T10:21:32Z

@@ -0,0 +1,107 @@
+# AsyncContext
+
+The `AsyncWorker` class may not be appropriate for every scenario, because with


Maybe could be good link the AsyncWorker doc

romandev

Thank you for review. I addressed your comments.

romandev · 2018-07-13T14:40:48Z

@@ -0,0 +1,107 @@
+# AsyncContext
+
+The `AsyncWorker` class may not be appropriate for every scenario, because with


mhdawson · 2018-07-18T16:58:51Z

+# AsyncContext
+
+The [AsyncWorker](async_worker.md) class may not be appropriate for every scenario, because with
+those the async execution still happens on the main event loop. When using any


I don't think this is quite right as the async execution happens in a thread from the existing thread pool. I'll have a closer look and add some I think describes the difference.

I matched up with https://nodejs.org/api/n-api.html#n_api_custom_asynchronous_operations.

romandev · 2018-09-06T00:34:22Z

@mhdawson @NickNaso Could you pleaser review this?

romandev · 2018-09-11T06:54:42Z

@mhdawson @NickNaso Gentle ping

NickNaso

Hi @romandev the code is good to me. Just some suggestions about the documentation. Recently we decided to use the complete namespace to refer at node-addon-api class. So please do another pass on documentation and add Napi namespace where necessary.

NickNaso · 2018-09-11T08:40:00Z

@@ -0,0 +1,148 @@
+# AsyncContext
+
+The [AsyncWorker](async_worker.md) class may not be appropriate for every


AsyncWorker should be Napi::AsyncWorker

NickNaso · 2018-09-11T08:40:28Z

+
+The [AsyncWorker](async_worker.md) class may not be appropriate for every
+scenario. When using any other async mechanism, introducing a new class 
+`AsyncContext`is necessary to ensure an async operation is properly tracked by


AsyncContext should be Napi::AsyncContext

NickNaso · 2018-09-11T08:40:55Z

+The [AsyncWorker](async_worker.md) class may not be appropriate for every
+scenario. When using any other async mechanism, introducing a new class 
+`AsyncContext`is necessary to ensure an async operation is properly tracked by
+the runtime. The `AsyncContext` class provides `MakeCallback()` method to


AsyncContext should be Napi::AsyncContext

NickNaso · 2018-09-11T08:42:21Z

+returning from an async operation (when there is no other script on the stack).
+
+```cpp
+Value MakeCallback() const


Add Napi namepsace

NickNaso · 2018-09-11T08:42:45Z

+Value MakeCallback() const
+```
+
+Returns a `Value` representing the JavaScript object returned.


Value should be Napi::Value

NickNaso · 2018-09-11T08:50:01Z

+Creates a new `AsyncContext`.
+
+```cpp
+explicit AsyncContext(const char* resource_name, const Function& callback);


Add Napi namespace

NickNaso · 2018-09-11T08:50:58Z

+- `[in] callback`: The function which will be called when an asynchronous
+operations ends.
+
+Returns an AsyncContext instance which can later make the given callback by


an AsyncContext should be a Napi::AsyncContext

NickNaso · 2018-09-11T08:51:39Z

+
+### Constructor
+
+Creates a new `AsyncContext`.


AsyncContext should be Napi::AsyncContext

NickNaso · 2018-09-11T08:51:51Z

+Creates a new `AsyncContext`.
+
+```cpp
+explicit AsyncContext(const char* resource_name, const Object& resource, const Function& callback);


Add Napi namespace

NickNaso · 2018-09-11T08:52:36Z

+- `[in] callback`: The function which will be called when an asynchronous
+operations ends.
+
+Returns an AsyncContext instance which can later make the given callback by


Returns an AsyncContext should be Returns a Napi::AsyncContext

NickNaso · 2018-09-17T21:52:10Z

@mhdawson Could you review this PR?

mhdawson · 2018-09-18T17:51:50Z

@romandev sorry its taken so long to comment.

A couple of high level comments/questions

It might be better if the class was not used with new/delete, but instead stack allocated and then automatically was cleaned up when existing a {} scope. I think that is the way that HandleScope works.
I'm not sure including passing a callback and including the MakeCallback methods makes sense. I think it would be possible to make 2 callbacks within a since AsyncContext, and if you already have a FunctionReference its just as easy to call MakeCallback on that? In particular, I think it is mostly necessary when you want to do more than a single call back into JavaScript before closing the scope and things like the micro-tick queue running.

romandev · 2018-09-27T18:02:33Z

@NickNaso, @mhdawson Sorry for delay. I've been recently busy for other works but I'll take this work in this weekend. I know that this is release blocker. If you don't mind, can you wait for me? or if you urgent, please feel free to take this.

mhdawson · 2018-09-27T18:03:13Z

@romandev if you can get to it this weekend I think it's worth waiting until next week for the release.

romandev · 2018-10-01T12:09:16Z

@mhdawson, @NickNaso I addressed all your comments. Please take another looks?

NickNaso

Some little fix to do for the rest it's ok to me.

NickNaso · 2018-10-01T12:59:15Z

 arguments of the function.
+- `[in] context`: Context for the async operation that is invoking the callback.
+This should normally be a value previously obtained from [Napi::AsyncContext](async_context.md).
+However `nullptr` is also allowed, which indicates the currenc async context


currenc -> current

NickNaso · 2018-10-01T12:59:34Z

 arguments of the function.
+- `[in] context`: Context for the async operation that is invoking the callback.
+This should normally be a value previously obtained from [Napi::AsyncContext](async_context.md).
+However `nullptr` is also allowed, which indicates the currenc async context


currenc -> current

NickNaso · 2018-10-01T12:59:53Z

 the arguments of the referenced function.
+- `[in] context`: Context for the async operation that is invoking the callback.
+This should normally be a value previously obtained from [Napi::AsyncContext](async_context.md).
+However `nullptr` is also allowed, which indicates the currenc async context


currenc -> current

NickNaso · 2018-10-01T13:00:15Z

 arguments of the referenced function.
+- `[in] context`: Context for the async operation that is invoking the callback.
+This should normally be a value previously obtained from [Napi::AsyncContext](async_context.md).
+However `nullptr` is also allowed, which indicates the currenc async context


currenc -> current

NickNaso · 2018-10-01T13:00:29Z

 arguments of the referenced function.
+- `[in] context`: Context for the async operation that is invoking the callback.
+This should normally be a value previously obtained from [Napi::AsyncContext](async_context.md).
+However `nullptr` is also allowed, which indicates the currenc async context


currenc -> current

This class provides a wrapper for the following custom asynchronous operation APIs. - napi_async_init() - napi_async_destroy()

romandev · 2018-10-01T14:58:07Z

FYI, I fixed typo as well :)

NickNaso

LGTM

NickNaso · 2018-10-01T15:04:33Z

@mhdawson Could you take a look at this PR?

mhdawson

LGTM

mhdawson · 2018-10-01T16:29:07Z

CI run to make sure its ok on 6.x https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api/784/

This class provides a wrapper for the following custom asynchronous operation APIs. - napi_async_init() - napi_async_destroy() PR-URL: #252 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>

mhdawson · 2018-10-01T16:54:27Z

Landed as dfcb939

This class provides a wrapper for the following custom asynchronous operation APIs. - napi_async_init() - napi_async_destroy() PR-URL: nodejs#252 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>

This class provides a wrapper for the following custom asynchronous operation APIs. - napi_async_init() - napi_async_destroy() PR-URL: nodejs/node-addon-api#252 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>

mhdawson force-pushed the master branch from 12fe6f9 to a750ed1 Compare May 8, 2018 19:30

romandev force-pushed the async_context branch from b272a22 to ad8b223 Compare July 11, 2018 06:35

NickNaso reviewed Jul 11, 2018

View reviewed changes

romandev force-pushed the async_context branch from ad8b223 to f2b4f2a Compare July 13, 2018 22:48

romandev commented Jul 13, 2018

View reviewed changes

romandev force-pushed the async_context branch 2 times, most recently from 8b13b94 to 4dc066e Compare July 13, 2018 22:53

mhdawson reviewed Jul 18, 2018

View reviewed changes

romandev force-pushed the async_context branch from 4dc066e to ef365f6 Compare September 6, 2018 00:31

NickNaso reviewed Sep 11, 2018

View reviewed changes

mhdawson mentioned this pull request Sep 18, 2018

Next release #347

Closed

6 tasks

romandev force-pushed the async_context branch from ef365f6 to aeab2b4 Compare October 1, 2018 12:07

NickNaso reviewed Oct 1, 2018

View reviewed changes

Implement AsyncContext class

0e6219b

This class provides a wrapper for the following custom asynchronous operation APIs. - napi_async_init() - napi_async_destroy()

romandev force-pushed the async_context branch from aeab2b4 to 0e6219b Compare October 1, 2018 14:26

NickNaso approved these changes Oct 1, 2018

View reviewed changes

mhdawson approved these changes Oct 1, 2018

View reviewed changes

mhdawson closed this Oct 1, 2018

mhdawson mentioned this pull request Oct 19, 2018

Fatal error with ObjectWrap since 1.5.0 #372

Closed

		@@ -0,0 +1,107 @@
		# AsyncContext

		The `AsyncWorker` class may not be appropriate for every scenario, because with

		@@ -0,0 +1,148 @@
		# AsyncContext

		The [AsyncWorker](async_worker.md) class may not be appropriate for every

Uh oh!

Conversation

romandev commented Apr 30, 2018

Uh oh!

mhdawson commented May 1, 2018

Uh oh!

mhdawson commented Jul 9, 2018

Uh oh!

romandev commented Jul 9, 2018

Uh oh!

romandev commented Jul 11, 2018

Uh oh!

NickNaso left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romandev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romandev commented Sep 6, 2018

Uh oh!

romandev commented Sep 11, 2018

Uh oh!

NickNaso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickNaso commented Sep 17, 2018

Uh oh!

mhdawson commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romandev commented Sep 27, 2018

Uh oh!

mhdawson commented Sep 27, 2018

Uh oh!

romandev commented Oct 1, 2018

Uh oh!

NickNaso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

NickNaso left a comment •

edited

Loading

mhdawson commented Sep 18, 2018 •

edited

Loading