From b583db38e89d1b38954fc920b6883a33d0bb0665 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Wed, 7 Feb 2018 15:40:55 -0800 Subject: [PATCH] [squash] remove new MakeCallback, AsyncInit, AsyncDestroy --- Makefile | 1 - doc/node_misc.md | 71 +++----------- nan.h | 141 ++++++++++------------------ test/binding.gyp | 4 - test/cpp/makecallbackcontext.cpp | 70 -------------- test/js/makecallbackcontext-test.js | 70 -------------- 6 files changed, 64 insertions(+), 293 deletions(-) delete mode 100644 test/cpp/makecallbackcontext.cpp delete mode 100644 test/js/makecallbackcontext-test.js diff --git a/Makefile b/Makefile index 6a0d7697..1b97e3f6 100644 --- a/Makefile +++ b/Makefile @@ -53,7 +53,6 @@ LINT_SOURCES = \ test/cpp/json-parse.cpp \ test/cpp/json-stringify.cpp \ test/cpp/makecallback.cpp \ - test/cpp/makecallbackcontext.cpp \ test/cpp/morenews.cpp \ test/cpp/multifile1.cpp \ test/cpp/multifile2.cpp \ diff --git a/doc/node_misc.md b/doc/node_misc.md index d8368f13..d024f85d 100644 --- a/doc/node_misc.md +++ b/doc/node_misc.md @@ -1,54 +1,20 @@ ## Miscellaneous Node Helpers - - Nan::AsyncInit() - - Nan::AsyncDestory() - Nan::AsyncResource - Nan::MakeCallback() - NAN_MODULE_INIT() - Nan::Export() - - -### Nan::AsyncInit() - -When calling back into JavaScript asynchornously, special care must be taken to ensure that the runtime can properly track -async hops. `Nan::AsyncInit` is an object that wraps `node::EmitAsyncInit` and returns a wrapper for the -`node::async_context` structure. The `async_context` can be provided to `Nan::MakeCallback` to properly restore the correct -async execution context. - -Signatures: - -```c++ -Nan::async_context AsyncInit(v8::MaybeLocal maybe_resource, - const char* name); -Nan::async_context AsyncInit(v8::MaybeLocal maybe_resource, - v8::Local name); -``` - -* `maybe_resource`: An optional object associated with the async work that will be passed to the possible [async_hooks][] - `init` hook. -* `name`: Identified for the kind of resource that is being provided for diagnostics information exposed by the [async_hooks][] - API. This will be passed to the possible `init` hook as the `type`. To avoid name collisions with other modules we recommend - that the name include the name of the owning module as a prefix. For example `mysql` module could use something like - `mysql:batch-db-query-resource`. -* An opaque `async_context` structure is returned. This should be passed to any `Nan::MakeCallback` operations done later. - -For more details, see the Node [async_hooks][] documentation. You might also want to take a look at the documentation for the -[N-API counterpart][napi]. For example usage, see the `makecallbackcontext.cpp` example in the `test/cpp` directory. - -Note: It might be more convenient to use `Nan::AsyncResource` instead of using this directly. - - -### Nan::AsyncDestroy() - -Wrapper around `node::EmitAsyncDestroy`. - -Note: It might be more convenient to use `Nan::AsyncResource` instead of using this directly. - ### Nan::AsyncResource -`Nan::AsyncResouce` is a convenience class that provides RAII wrapper around `Nan::AsyncInit`, `Nan::AsyncDestroy` and `Nan::MakeCallback`. It is analogous to the `AsyncResource` JavaScript class exposed by Node's [async_hooks][] API. +This class is analogous to the `AsyncResource` JavaScript class exposed by Node's [async_hooks][] API. + +When calling back into JavaScript asynchornously, special care must be taken to ensure that the runtime can properly track +async hops. `Nan::AsyncResource` is a class that provides an RAII wrapper around `node::EmitAsyncInit`, `node::EmitAsyncDestroy`, +and `node::MakeCallback`. Using this mechanism to call back into JavaScript, as opposed to `Nan::MakeCallback` or +`v8::Function::Call` ensures that the callback is executed in the correct async context. This ensures that async mechanisms +such as domains and [async_hooks][] function correctly. Definition: @@ -76,6 +42,7 @@ class AsyncResource { Nan::async_context async_context); }; ``` + * `maybe_resource`: An optional object associated with the async work that will be passed to the possible [async_hooks][] `init` hook. * `name`: Identified for the kind of resource that is being provided for diagnostics information exposed by the [async_hooks][] @@ -86,7 +53,8 @@ class AsyncResource { correct async execution context. * `AsyncDestroy` is automatically called when an AsyncResource object is destroyed. -For example usage, see the `asyncresource.cpp` example in the `test/cpp` directory. +For more details, see the Node [async_hooks][] documentation. You might also want to take a look at the documentation for the +[N-API counterpart][napi]. For example usage, see the `asyncresource.cpp` example in the `test/cpp` directory. ### Nan::MakeCallback() @@ -100,23 +68,8 @@ Use `MakeCallback()` rather than using `v8::Function#Call()` directly in order t Signatures: ```c++ -v8::MaybeLocal Nan::MakeCallback(v8::Local target, - v8::Local func, - int argc, - v8::Local* argv, - Nan::async_context async_context); -v8::MaybeLocal Nan::MakeCallback(v8::Local target, - v8::Local symbol, - int argc, - v8::Local* argv, - Nan::async_context async_context); -v8::MaybeLocal Nan::MakeCallback(v8::Local target, - const char* method, - int argc, - v8::Local* argv, - Nan::async_context async_context); - -// Legacy versions. We recommend the async context preserving versions above. +// Legacy versions. We recommend the AsyncResource class and +// AsyncResource::runInAsyncScope instead. v8::Local Nan::MakeCallback(v8::Local target, v8::Local func, int argc, diff --git a/nan.h b/nan.h index bb0e95d3..ae4894f3 100644 --- a/nan.h +++ b/nan.h @@ -1273,7 +1273,7 @@ class Utf8String { #endif // NODE_MODULE_VERSION -//=== async_context and context aware MakeCallback ============================= +//=== async_context ============================================================ #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION typedef node::async_context async_context; @@ -1281,87 +1281,6 @@ class Utf8String { struct async_context {}; #endif - inline async_context AsyncInit( - MaybeLocal maybe_resource - , v8::Local resource_name) { -#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION - return async_context(); -#else - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - - v8::Local resource = - maybe_resource.IsEmpty() ? New() - : maybe_resource.ToLocalChecked(); - - node::async_context context = - node::EmitAsyncInit(isolate, resource, resource_name); - return static_cast(context); -#endif - } - - inline async_context AsyncInit( - MaybeLocal maybe_resource - , const char* name) { - return AsyncInit(maybe_resource, New(name).ToLocalChecked()); - } - - inline void AsyncDestroy(async_context context) { -#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - node::async_context node_context = - static_cast(context); - node::EmitAsyncDestroy(isolate, node_context); -#endif - } - - inline MaybeLocal MakeCallback( - v8::Local target - , v8::Local func - , int argc - , v8::Local* argv - , async_context asyncContext) { -#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION - // Ignore the async_context value. - return MakeCallback(target, func, argc, argv); -#else - return node::MakeCallback( - v8::Isolate::GetCurrent(), target, func, argc, argv, - static_cast(asyncContext)); -#endif - } - - inline MaybeLocal MakeCallback( - v8::Local target - , v8::Local symbol - , int argc - , v8::Local* argv - , async_context asyncContext) { -#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION - // Ignore the async_context value. - return MakeCallback(target, symbol, argc, argv); -#else - return node::MakeCallback( - v8::Isolate::GetCurrent(), target, symbol, argc, argv, - static_cast(asyncContext)); -#endif - } - - inline MaybeLocal MakeCallback( - v8::Local target - , const char* method - , int argc - , v8::Local* argv - , async_context asyncContext) { -#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION - // Ignore the async_context value. - return MakeCallback(target, method, argc, argv); -#else - return node::MakeCallback( - v8::Isolate::GetCurrent(), target, method, argc, argv, - static_cast(asyncContext)); -#endif - } - // === AsyncResource =========================================================== class AsyncResource { @@ -1369,15 +1288,41 @@ class Utf8String { AsyncResource( MaybeLocal maybe_resource , v8::Local resource_name) { - asyncContext = AsyncInit(maybe_resource, resource_name); +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + + v8::Local resource = + maybe_resource.IsEmpty() ? New() + : maybe_resource.ToLocalChecked(); + + node::async_context context = + node::EmitAsyncInit(isolate, resource, resource_name); + asyncContext = static_cast(context); +#endif } AsyncResource(MaybeLocal maybe_resource, const char* name) { - asyncContext = AsyncInit(maybe_resource, name); +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + + v8::Local resource = + maybe_resource.IsEmpty() ? New() + : maybe_resource.ToLocalChecked(); + v8::Local name_string = + New(name).ToLocalChecked(); + node::async_context context = + node::EmitAsyncInit(isolate, resource, name_string); + asyncContext = static_cast(context); +#endif } ~AsyncResource() { - AsyncDestroy(asyncContext); +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + node::async_context node_context = + static_cast(asyncContext); + node::EmitAsyncDestroy(isolate, node_context); +#endif } inline MaybeLocal runInAsyncScope( @@ -1385,7 +1330,13 @@ class Utf8String { , v8::Local func , int argc , v8::Local* argv) { - return MakeCallback(target, func, argc, argv, asyncContext); +#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION + return MakeCallback(target, func, argc, argv); +#else + return node::MakeCallback( + v8::Isolate::GetCurrent(), target, func, argc, argv, + static_cast(asyncContext)); +#endif } inline MaybeLocal runInAsyncScope( @@ -1393,7 +1344,13 @@ class Utf8String { , v8::Local symbol , int argc , v8::Local* argv) { - return MakeCallback(target, symbol, argc, argv, asyncContext); +#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION + return MakeCallback(target, symbol, argc, argv); +#else + return node::MakeCallback( + v8::Isolate::GetCurrent(), target, symbol, argc, argv, + static_cast(asyncContext)); +#endif } inline MaybeLocal runInAsyncScope( @@ -1401,10 +1358,16 @@ class Utf8String { , const char* method , int argc , v8::Local* argv) { - return MakeCallback(target, method, argc, argv, asyncContext); +#if NODE_MODULE_VERSION < NODE_8_0_MODULE_VERSION + return MakeCallback(target, method, argc, argv); +#else + return node::MakeCallback( + v8::Isolate::GetCurrent(), target, method, argc, argv, + static_cast(asyncContext)); +#endif } - protected: + private: async_context asyncContext; }; diff --git a/test/binding.gyp b/test/binding.gyp index f0c78089..28d51a4a 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -90,10 +90,6 @@ "target_name" : "makecallback" , "sources" : [ "cpp/makecallback.cpp" ] } - , { - "target_name" : "makecallbackcontext" - , "sources" : [ "cpp/makecallbackcontext.cpp" ] - } , { "target_name" : "asyncresource" , "sources" : [ "cpp/asyncresource.cpp" ] diff --git a/test/cpp/makecallbackcontext.cpp b/test/cpp/makecallbackcontext.cpp deleted file mode 100644 index a9543b67..00000000 --- a/test/cpp/makecallbackcontext.cpp +++ /dev/null @@ -1,70 +0,0 @@ -/********************************************************************* - * NAN - Native Abstractions for Node.js - * - * Copyright (c) 2018 NAN contributors - * - * MIT License - ********************************************************************/ - -#include -#include - -using namespace Nan; // NOLINT(build/namespaces) - -class DelayRequest { - public: - DelayRequest(int milliseconds_, v8::Local callback_) - : milliseconds(milliseconds_) { - callback.Reset(callback_); - request.data = this; - asyncContext = AsyncInit(MaybeLocal(), - "nan:test.DelayRequest"); - } - ~DelayRequest() { - AsyncDestroy(asyncContext); - callback.Reset(); - } - - Persistent callback; - uv_work_t request; - async_context asyncContext; - int milliseconds; -}; - -void Delay(uv_work_t* req) { - DelayRequest *delay_request = static_cast(req->data); - sleep(delay_request->milliseconds / 1000); -} - -void AfterDelay(uv_work_t* req, int status) { - HandleScope scope; - - DelayRequest *delay_request = static_cast(req->data); - v8::Local callback = New(delay_request->callback); - v8::Local argv[0] = {}; - - v8::Local target = New(); - MakeCallback(target, callback, 0, argv, delay_request->asyncContext); - - delete delay_request; -} - -NAN_METHOD(Delay) { - int delay = To(info[0]).FromJust(); - v8::Local cb = To(info[1]).ToLocalChecked(); - - DelayRequest* delay_request = new DelayRequest(delay, cb); - - uv_queue_work( - uv_default_loop() - , &delay_request->request - , Delay - , reinterpret_cast(AfterDelay)); -} - -NAN_MODULE_INIT(Init) { - Set(target, New("delay").ToLocalChecked(), - GetFunction(New(Delay)).ToLocalChecked()); -} - -NODE_MODULE(makecallbackcontext, Init) diff --git a/test/js/makecallbackcontext-test.js b/test/js/makecallbackcontext-test.js deleted file mode 100644 index 6576975b..00000000 --- a/test/js/makecallbackcontext-test.js +++ /dev/null @@ -1,70 +0,0 @@ -/********************************************************************* - * NAN - Native Abstractions for Node.js - * - * Copyright (c) 2018 NAN contributors - * - * MIT License - ********************************************************************/ - -try { - require('async_hooks'); -} catch (e) { - process.exit(0); -} - -const test = require('tap').test - , testRoot = require('path').resolve(__dirname, '..') - , delay = require('bindings')({ module_root: testRoot, bindings: 'makecallbackcontext' }).delay - , asyncHooks = require('async_hooks'); - -test('makecallbackcontext', function (t) { - t.plan(7); - - var resourceAsyncId; - var originalExecutionAsyncId; - var beforeCalled = false; - var afterCalled = false; - var destroyCalled = false; - - var hooks = asyncHooks.createHook({ - init: function(asyncId, type, triggerAsyncId, resource) { - if (type === 'nan:test.DelayRequest') { - resourceAsyncId = asyncId; - } - }, - before: function(asyncId) { - if (asyncId === resourceAsyncId) { - beforeCalled = true; - } - }, - after: function(asyncId) { - if (asyncId === resourceAsyncId) { - afterCalled = true; - } - }, - destroy: function(asyncId) { - if (asyncId === resourceAsyncId) { - destroyCalled = true; - } - } - - }); - hooks.enable(); - - originalExecutionAsyncId = asyncHooks.executionAsyncId(); - delay(1000, function() { - t.equal(asyncHooks.executionAsyncId(), resourceAsyncId, - 'callback should have the correct execution context'); - t.equal(asyncHooks.triggerAsyncId(), originalExecutionAsyncId, - 'callback should have the correct trigger context'); - t.ok(beforeCalled, 'before should have been called'); - t.notOk(afterCalled, 'after should not have been called yet'); - setTimeout(function() { - t.ok(afterCalled, 'after should have been called'); - t.ok(destroyCalled, 'destroy should have been called'); - t.equal(asyncHooks.triggerAsyncId(), resourceAsyncId, - 'setTimeout should have been triggered by the async resource'); - hooks.disable(); - }, 1); - }); -});