Implement AsyncContext class #252

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.

README.md

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

doc/async_context.md

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

README.md

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

doc/async_context.md

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


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

doc/async_context.md

+# 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

doc/async_context.md

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

doc/async_context.md

+
+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

doc/async_context.md

+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

doc/async_context.md

+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

doc/async_context.md

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


Value should be Napi::Value

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

doc/async_context.md

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


Add Napi namespace

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

doc/async_context.md

+- `[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

doc/async_context.md

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


AsyncContext should be Napi::AsyncContext

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

doc/async_context.md

+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

doc/async_context.md

+- `[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.

doc/function.md

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

doc/function.md

 ```

 - `[in] recv`: The `this` object passed to the called function.
 - `[in] args`: List of JavaScript values as `napi_value` representing the
 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

doc/function.md

 ```

 - `[in] recv`: The `this` object passed to the called function.
 - `[in] argc`: The number of the arguments passed to the function.
 - `[in] args`: Array of JavaScript values as `napi_value` representing the
 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

doc/function_reference.md

 ```

 - `[in] recv`: The `this` object passed to the referenced function when it's called.
 - `[in] args`: Initializer list of JavaScript values as `napi_value` representing
 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

doc/function_reference.md

 ```

 - `[in] recv`: The `this` object passed to the referenced function when it's called.
 - `[in] args`: Vector of JavaScript values as `napi_value` representing 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:29Z

doc/function_reference.md

 ```

 - `[in] recv`: The `this` object passed to the referenced function when it's called.
 - `[in] argc`: The number of arguments passed to the referenced function.
 - `[in] args`: Array of JavaScript values as `napi_value` representing 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

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 <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>

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 <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>

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 <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>

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

Implement AsyncContext class #252

Implement AsyncContext class #252

Conversation

romandev commented Apr 30, 2018

mhdawson commented May 1, 2018

mhdawson commented Jul 9, 2018

romandev commented Jul 9, 2018

romandev commented Jul 11, 2018

NickNaso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romandev 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

romandev commented Sep 6, 2018

romandev commented Sep 11, 2018

NickNaso 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

NickNaso commented Sep 17, 2018

mhdawson commented Sep 18, 2018 • edited Loading

romandev commented Sep 27, 2018

mhdawson commented Sep 27, 2018

romandev commented Oct 1, 2018

NickNaso 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

romandev commented Oct 1, 2018

NickNaso left a comment

Choose a reason for hiding this comment

NickNaso commented Oct 1, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Oct 1, 2018

mhdawson commented Oct 1, 2018

NickNaso left a comment •

edited

Loading

mhdawson commented Sep 18, 2018 •

edited

Loading