introduce AsyncResource class

[ofrobots]

This commit adds support for the async context accepting versions of
node::MakeCallback. An async_context concept has been added as a
wrapper around node::async_context, along with APIs for initializing
and destroying async context, similar to how N-API exposes this
functionality.

Ref: nodejs/node#13254

[kkoopa]

I took a quick look and this looks mostly good. I will take a closer look later and see if I can get the oldest versions working. The old Nan::MakeCallback can be marked with NAN_DEPRECATED.

[ofrobots]

I was planning on a separate PR with the deprecations. What is this project's stance on whether marking something as deprecated is semver-major? If that is considered okay for a semver-minor, I can include those changes here.

Note that intend to follow-up with other PRs that add similar support to Nan::Callback and Nan::AsyncWorker class hierarchy. My thinking was that we should provide all the new features to developers before starting to mark things as deprecated; but I can do things differently if you prefer. Thanks in advance for reviewing :).

[kkoopa]

Deprecation with warnings is fine in minor updates, with the actual removal happening in a subsequent major release. I agree that all functionality should be available before marking as deprecated, but since deprecations with suitable replacements are fine in minor updates, that should happen at the same time.

[ofrobots]

The problem is that marking the legacy MakeCallback as deprecated will lead to quite a bit of warnings unless I go and modify all the tests as well. The tests will probably require the changes to Nan::Callback and AsyncWorker. That would balloon the scope of this PR; something I would like to avoid.

Alternatively, we can add NAN_DEPRECATED now, and live with the warnings until the subsequent PRs are landed. Or we could do it in stages as I original proposed. Whatever works for you.

[kkoopa]

Ah, yes. In that case I think your original plan of doing the deprecation separately is the best.

…

On February 7, 2018 2:18:57 AM GMT+02:00, Ali Ijaz Sheikh ***@***.***> wrote: The problem is that marking the legacy MakeCallback as deprecated will lead to quite a bit of warnings unless I go and modify all the tests as well. The tests will probably require the changes to `Nan::Callback` and `AsyncWorker`. That would balloon the scope of this PR; something I would like to avoid. Alternatively, we can add NAN_DEPRECATED now, and live with the warnings until the subsequent PRs are landed. Or we could do it in stages as I original proposed. Whatever works for you.

[ofrobots]

I'm getting compile problems with Node <=0.12 without my changes:

❯   CXX(target) Release/obj.target/accessors/cpp/accessors.o
In file included from ../cpp/accessors.cpp:9:
In file included from ../../nan.h:51:
In file included from /Users/ofrobots/.node-gyp/0.12.18/include/node/node.h:61:
/Users/ofrobots/.node-gyp/0.12.18/include/node/v8.h:5800:54: error: 'CreateHandle' is a protected member of 'v8::HandleScope'
  return Handle<T>(reinterpret_cast<T*>(HandleScope::CreateHandle(
                                        ~~~~~~~~~~~~~^~~~~~~~~~~~

Known issue? I'll go take care of the rest that I caused :).

[kkoopa]

I think I have seen that error in some test run in the last three months, but only on OS X and only 0.12. It has definitely passed before. Based on the lines and error messages, I think I wrote it off as a random fluke or such with that particular V8 version and OS X, but it seems to need closer investigation.

[kkoopa]

These are the lines in question:
https://github.com/nodejs/nan/blob/master/test/cpp/accessors.cpp#L9

#include <nan.h>

https://github.com/nodejs/nan/blob/master/nan.h#L51

#include <node.h>

https://github.com/nodejs/node/blob/v0.12/src/node.h#L61

#include "v8.h"  // NOLINT(build/include_order)

https://github.com/nodejs/node/blob/v0.12/deps/v8/include/v8.h#L5800

return Handle<T>(reinterpret_cast<T*>(HandleScope::CreateHandle(

Based on this, I guess the issue is in that old version of V8 or in that version of clang.

[ofrobots]

I can reliably reproduce it on my machine; I suspect that the compiler being used now is newer and it no longer likes the code it sees. The compiler is indeed making a valid complaint about the contents of v8.h. The work-around would be a single line change in v8.h (make Handle a friend class of HandleScope – it is not enough for the subclass Local to be a friend).

I am not sure how we can actually make the change given that neither V8 nor Node are going to do a release for that version line :/.

[kkoopa]

I think the only recourse is to document it as a known issue and stop testing 0.12 on OS X. If someone needs it built, they have to get an older compiler or build Node from source with a patched V8.

[kkoopa]

This seems to pass all test now (modulo 0.12 on OS X). I looked through the code again and everything seems good.

One thing did come to mind: I am not familiar with the new async contexts, so I do not know if it was iterated there, but would it make sense to use RAII and have an AsyncContext object instead of, or in addition to, the AsyncInit AsyncDestroy pair? It seems a bit easy to forget an AsyncDestroy.

[ofrobots]

@kkoopa 👍 . Added a AsyncResource class as an additional convenience RAII wrapper. Based on discussion in nodejs/node#18513, we are preferring the name runInAsyncScope over MakeCallback as being more meaningful.

I am still debating whether we should continue to expose the lower-level AsyncInit, AsyncDestroy, MakeCallback. Thoughts?

[kkoopa]

Assuming that AsyncInit and AsyncDestroy always should be matched, I cannot quickly think of a reason to ever use them instead of AsyncResource. As for MakeCallback , I would be inclined to leave it in --mostly for making a slightly easier upgrade path with a simple search/replace, but I am not entirely sure if that actually is beneficial enough.

[ofrobots]

The new MakeCallback isn't really useful without AsyncInit and AsyncDestroy because the user would be forced to use AsyncResource anyway. I agree that it is a slightly easier upgrade path to have the new MakeCallbacks.

Perhaps the documentation will be enough to guide users towards AsyncResource and AsyncResource::runInAsyncScope? Unsure, but leaning slightly towards removing AsyncInit, AsyncDestroy and the new MakeCallbacks.

[kkoopa]

Documentation would probably be enough, but after pondering this some more I agree that it is best to remove them all. They can always be added back if someone files a ticket for a valid use case which cannot be done with AsyncResource.

[ofrobots]

The CI is green.

Thanks for all your reviews on this!

[kkoopa]

Thanks for all your work coding this. I'll look at merging tomorrow.

…

On February 8, 2018 3:51:34 AM GMT+02:00, Ali Ijaz Sheikh ***@***.***> wrote: The CI is green. Thanks for all your reviews on this!