From 56c282117a4a37aef8b6e8bca55d6106d74552d4 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 8 Feb 2018 16:31:44 -0800 Subject: [PATCH 1/9] Add context aware Nan::Callback::Call functions --- Makefile | 1 + doc/callback.md | 23 ++++++++++- nan.h | 73 +++++++++++++++++++++++++++++++-- test/binding.gyp | 4 ++ test/cpp/callbackcontext.cpp | 66 +++++++++++++++++++++++++++++ test/js/callbackcontext-test.js | 73 +++++++++++++++++++++++++++++++++ 6 files changed, 235 insertions(+), 5 deletions(-) create mode 100644 test/cpp/callbackcontext.cpp create mode 100644 test/js/callbackcontext-test.js diff --git a/Makefile b/Makefile index 1b97e3f6..9e8a2a3a 100644 --- a/Makefile +++ b/Makefile @@ -48,6 +48,7 @@ LINT_SOURCES = \ test/cpp/error.cpp \ test/cpp/gc.cpp \ test/cpp/indexedinterceptors.cpp \ + test/cpp/callbackcontext.cpp \ test/cpp/converters.cpp \ test/cpp/isolatedata.cpp \ test/cpp/json-parse.cpp \ diff --git a/doc/callback.md b/doc/callback.md index 46de1d71..dc862068 100644 --- a/doc/callback.md +++ b/doc/callback.md @@ -22,11 +22,13 @@ class Callback { v8::Local operator*() const; - v8::Local operator()(v8::Local target, + v8::Local operator()(AsyncResource* async_resource, + v8::Local target, int argc = 0, v8::Local argv[] = 0) const; - v8::Local operator()(int argc = 0, + v8::Local operator()(AsyncResource* async_resource, + int argc = 0, v8::Local argv[] = 0) const; void SetFunction(const v8::Local &fn); @@ -39,6 +41,23 @@ class Callback { void Reset(); + v8::Local Call(v8::Local target, + int argc, + v8::Local argv[], + AsyncResource* async_resource) const; + v8::Local Call(int argc, + v8::Local argv[], + AsyncResource* async_resource) const; + + // Legacy versions. Use the versions that accept an async_resource instead + // as they run the callback in the correct async context as specified by the + // resource. + v8::Local operator()(v8::Local target, + int argc = 0, + v8::Local argv[] = 0) const; + + v8::Local operator()(int argc = 0, + v8::Local argv[] = 0) const; v8::Local Call(v8::Local target, int argc, v8::Local argv[]) const; diff --git a/nan.h b/nan.h index ae4894f3..920178c6 100644 --- a/nan.h +++ b/nan.h @@ -1500,6 +1500,21 @@ class Callback { return this->Call(target, argc, argv); } + inline v8::Local operator()( + AsyncResource* resource + , int argc = 0 + , v8::Local argv[] = 0) const { + return this->Call(argc, argv, resource); + } + + inline v8::Local operator()( + AsyncResource* resource + , v8::Local target + , int argc = 0 + , v8::Local argv[] = 0) const { + return this->Call(target, argc, argv, resource); + } + inline v8::Local operator()( int argc = 0 , v8::Local argv[] = 0) const { @@ -1531,7 +1546,7 @@ class Callback { Call(v8::Local target , int argc , v8::Local argv[]) const { -#if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) +#if NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION v8::Isolate *isolate = v8::Isolate::GetCurrent(); return Call_(isolate, target, argc, argv); #else @@ -1541,7 +1556,45 @@ class Callback { inline v8::Local Call(int argc, v8::Local argv[]) const { -#if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) +#if NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION + v8::Isolate *isolate = v8::Isolate::GetCurrent(); + v8::EscapableHandleScope scope(isolate); + return scope.Escape( + Call_(isolate, isolate->GetCurrentContext()->Global(), argc, argv)); +#else + v8::HandleScope scope; + return scope.Close(Call_(v8::Context::GetCurrent()->Global(), argc, argv)); +#endif + } + + inline v8::Local + Call(v8::Local target + , int argc + , v8::Local argv[] + , AsyncResource* resource) const { +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + + return Call_(isolate, target, argc, argv, resource); +#elif NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION + v8::Isolate *isolate = v8::Isolate::GetCurrent(); + return Call_(isolate, target, argc, argv); +#else + return Call_(target, argc, argv); +#endif + } + + inline v8::Local + Call(int argc, v8::Local argv[], AsyncResource* resource) const { +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::EscapableHandleScope scope(isolate); + return scope.Escape(Call_(isolate + , isolate->GetCurrentContext()->Global() + , argc + , argv + , resource)); +#elif NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION v8::Isolate *isolate = v8::Isolate::GetCurrent(); v8::EscapableHandleScope scope(isolate); return scope.Escape( @@ -1556,7 +1609,21 @@ class Callback { NAN_DISALLOW_ASSIGN_COPY_MOVE(Callback) Persistent handle_; -#if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + v8::Local Call_(v8::Isolate *isolate + , v8::Local target + , int argc + , v8::Local argv[] + , AsyncResource* resource) const { + EscapableHandleScope scope; + + v8::Local func = New(handle_); + return scope.Escape(resource->runInAsyncScope(target, func, argc, argv) + .ToLocalChecked()); + } +#endif + +#if NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION v8::Local Call_(v8::Isolate *isolate , v8::Local target , int argc diff --git a/test/binding.gyp b/test/binding.gyp index 28d51a4a..c2928357 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -94,6 +94,10 @@ "target_name" : "asyncresource" , "sources" : [ "cpp/asyncresource.cpp" ] } + , { + "target_name" : "callbackcontext" + , "sources" : [ "cpp/callbackcontext.cpp" ] + } , { "target_name" : "isolatedata" , "sources" : [ "cpp/isolatedata.cpp" ] diff --git a/test/cpp/callbackcontext.cpp b/test/cpp/callbackcontext.cpp new file mode 100644 index 00000000..14cc8293 --- /dev/null +++ b/test/cpp/callbackcontext.cpp @@ -0,0 +1,66 @@ +/********************************************************************* + * NAN - Native Abstractions for Node.js + * + * Copyright (c) 2018 NAN contributors + * + * MIT License + ********************************************************************/ + +#include +#include + +using namespace Nan; // NOLINT(build/namespaces) + +class DelayRequest : public AsyncResource { + public: + DelayRequest(int milliseconds_, v8::Local callback_) + : AsyncResource(MaybeLocal(), "nan:test.DelayRequest"), + callback(callback_), + milliseconds(milliseconds_) { + request.data = this; + } + ~DelayRequest() {} + + Callback callback; + uv_work_t request; + 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 argv[0] = {}; + + v8::Local target = New(); + + // Run the callback in the async context. + delay_request->callback.Call(target, 0, argv, delay_request); + + 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(asyncresource, Init) diff --git a/test/js/callbackcontext-test.js b/test/js/callbackcontext-test.js new file mode 100644 index 00000000..8854e47a --- /dev/null +++ b/test/js/callbackcontext-test.js @@ -0,0 +1,73 @@ +/********************************************************************* + * NAN - Native Abstractions for Node.js + * + * Copyright (c) 2018 NAN contributors + * + * MIT License + ********************************************************************/ + +const bindingName = 'callbackcontext'; + +try { + require('async_hooks'); +} catch (e) { + console.log('1..0 # Skipped: ' + bindingName); + process.exit(0); +} + +const test = require('tap').test + , testRoot = require('path').resolve(__dirname, '..') + , delay = require('bindings')({ module_root: testRoot, bindings: bindingName }).delay + , asyncHooks = require('async_hooks'); + +test(bindingName, 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); + }); +}); From dad30e87d0f350bcce8d900eaf5d81462c6898b7 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 8 Feb 2018 17:11:50 -0800 Subject: [PATCH 2/9] Make AsyncWorker context aware --- doc/asyncworker.md | 15 ++++++--- nan.h | 39 +++++++++++++++------- test/cpp/asyncworker.cpp | 3 +- test/js/asyncworker-test.js | 65 ++++++++++++++++++++++++++++++++++++- 4 files changed, 104 insertions(+), 18 deletions(-) diff --git a/doc/asyncworker.md b/doc/asyncworker.md index b75cb4be..5cfc77c4 100644 --- a/doc/asyncworker.md +++ b/doc/asyncworker.md @@ -12,12 +12,17 @@ `Nan::AsyncWorker` is an _abstract_ class that you can subclass to have much of the annoying asynchronous queuing and handling taken care of for you. It can even store arbitrary V8 objects for you and have them persist while the asynchronous work is in progress. +This class internally handles the details of creating an [`AsyncResource`][AsyncResource], and running the callback in the +correct async context. To be able to identify the async resources created by this class in async-hooks, provide a +`resource_name` to the constructor. It is recommended that the module name be used as a prefix to the `resource_name` to avoid +collisions in the names. For more details see [`AsyncResource`][AsyncResource] documentation. The `resource_name` needs to stay valid for the lifetime of worker instance. + Definition: ```c++ class AsyncWorker { public: - explicit AsyncWorker(Callback *callback_); + explicit AsyncWorker(Callback *callback_, const char* resource_name = "nan:AsyncWorker"); virtual ~AsyncWorker(); @@ -73,9 +78,9 @@ Definition: template class AsyncProgressWorkerBase : public AsyncWorker { public: - explicit AsyncProgressWorker(Callback *callback_); + explicit AsyncProgressWorkerBase(Callback *callback_, const char* resource_name = ...); - virtual ~AsyncProgressWorker(); + virtual ~AsyncProgressWorkerBase(); void WorkProgress(); @@ -108,7 +113,7 @@ Definition: template class AsyncProgressQueueWorker : public AsyncWorker { public: - explicit AsyncProgressQueueWorker(Callback *callback_); + explicit AsyncProgressQueueWorker(Callback *callback_, const char* resource_name = "nan:AsyncProgressQueueWorker"); virtual ~AsyncProgressQueueWorker(); @@ -137,3 +142,5 @@ Definition: ```c++ void AsyncQueueWorker(AsyncWorker *); ``` + +[AsyncResource]: "node_misc.html#api_nan_asyncresource" diff --git a/nan.h b/nan.h index 920178c6..b0a2018d 100644 --- a/nan.h +++ b/nan.h @@ -1668,13 +1668,15 @@ class Callback { /* abstract */ class AsyncWorker { public: - explicit AsyncWorker(Callback *callback_) + explicit AsyncWorker(Callback *callback_, + const char* resource_name = "nan:AsyncWorker") : callback(callback_), errmsg_(NULL) { request.data = this; HandleScope scope; v8::Local obj = New(); persistentHandle.Reset(obj); + async_resource = new AsyncResource(obj, resource_name); } virtual ~AsyncWorker() { @@ -1684,6 +1686,7 @@ class Callback { persistentHandle.Reset(); delete callback; delete[] errmsg_; + delete async_resource; } virtual void WorkComplete() { @@ -1743,11 +1746,12 @@ class Callback { protected: Persistent persistentHandle; Callback *callback; + AsyncResource *async_resource; virtual void HandleOKCallback() { HandleScope scope; - callback->Call(0, NULL); + callback->Call(0, NULL, async_resource); } virtual void HandleErrorCallback() { @@ -1756,7 +1760,7 @@ class Callback { v8::Local argv[] = { v8::Exception::Error(New(ErrorMessage()).ToLocalChecked()) }; - callback->Call(1, argv); + callback->Call(1, argv, async_resource); } void SetErrorMessage(const char *msg) { @@ -1778,8 +1782,10 @@ class Callback { /* abstract */ class AsyncBareProgressWorkerBase : public AsyncWorker { public: - explicit AsyncBareProgressWorkerBase(Callback *callback_) - : AsyncWorker(callback_) { + explicit AsyncBareProgressWorkerBase( + Callback *callback_, + const char* resource_name = "nan:AsyncBareProgressWorkerBase") + : AsyncWorker(callback_, resource_name) { uv_async_init( uv_default_loop() , &async @@ -1818,8 +1824,10 @@ template /* abstract */ class AsyncBareProgressWorker : public AsyncBareProgressWorkerBase { public: - explicit AsyncBareProgressWorker(Callback *callback_) - : AsyncBareProgressWorkerBase(callback_) { + explicit AsyncBareProgressWorker( + Callback *callback_, + const char* resource_name = "nan:AsyncBareProgressWorker") + : AsyncBareProgressWorkerBase(callback_, resource_name) { } virtual ~AsyncBareProgressWorker() { @@ -1858,8 +1866,11 @@ template /* abstract */ class AsyncProgressWorkerBase : public AsyncBareProgressWorker { public: - explicit AsyncProgressWorkerBase(Callback *callback_) - : AsyncBareProgressWorker(callback_), asyncdata_(NULL), asyncsize_(0) { + explicit AsyncProgressWorkerBase( + Callback *callback_, + const char* resource_name = "nan:AsyncProgressWorkerBase") + : AsyncBareProgressWorker(callback_, resource_name), asyncdata_(NULL), + asyncsize_(0) { uv_mutex_init(&async_lock); } @@ -1914,8 +1925,10 @@ template /* abstract */ class AsyncBareProgressQueueWorker : public AsyncBareProgressWorkerBase { public: - explicit AsyncBareProgressQueueWorker(Callback *callback_) - : AsyncBareProgressWorkerBase(callback_) { + explicit AsyncBareProgressQueueWorker( + Callback *callback_, + const char* resource_name = "nan:AsyncBareProgressQueueWorker") + : AsyncBareProgressWorkerBase(callback_, resource_name) { } virtual ~AsyncBareProgressQueueWorker() { @@ -1951,7 +1964,9 @@ template /* abstract */ class AsyncProgressQueueWorker : public AsyncBareProgressQueueWorker { public: - explicit AsyncProgressQueueWorker(Callback *callback_) + explicit AsyncProgressQueueWorker( + Callback *callback_, + const char* resource_name = "nan:AsyncProgressQueueWorker") : AsyncBareProgressQueueWorker(callback_) { uv_mutex_init(&async_lock); } diff --git a/test/cpp/asyncworker.cpp b/test/cpp/asyncworker.cpp index 90a2b3dd..36822d46 100644 --- a/test/cpp/asyncworker.cpp +++ b/test/cpp/asyncworker.cpp @@ -17,7 +17,8 @@ using namespace Nan; // NOLINT(build/namespaces) class SleepWorker : public AsyncWorker { public: SleepWorker(Callback *callback, int milliseconds) - : AsyncWorker(callback), milliseconds(milliseconds) {} + : AsyncWorker(callback, "nan:test.SleepWorker"), + milliseconds(milliseconds) {} ~SleepWorker() {} void Execute () { diff --git a/test/js/asyncworker-test.js b/test/js/asyncworker-test.js index 1d4d60cd..146df20d 100644 --- a/test/js/asyncworker-test.js +++ b/test/js/asyncworker-test.js @@ -26,4 +26,67 @@ test('asyncworker', function (t) { t.ok(ticks > 6, 'got plenty of ticks! (' + ticks + ')') t.end() }) -}) +}); + +test('asyncworker context', function (t) { + var asyncHooks; + try { + asyncHooks = require('async_hooks'); + } catch (e) { + t.ok(true); + t.end(); + return; + } + + t.plan(7); + + var sleep = bindings.a; + 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.SleepWorker') { + 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(); + sleep(200, 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); + }); +}); + From 2678eaf2d89c8e4fa54eb9ecc77fe254d057b1b9 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 8 Feb 2018 17:24:21 -0800 Subject: [PATCH 3/9] Make AsyncResource non-{copy,assign,move}-able Also some cleanup * avoid unused private field warning on old node versions * no reason to have async_context as a concept * avoids camelCase for field asyncContext less camel, less case --- nan.h | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/nan.h b/nan.h index b0a2018d..54a8d598 100644 --- a/nan.h +++ b/nan.h @@ -1273,14 +1273,6 @@ class Utf8String { #endif // NODE_MODULE_VERSION -//=== async_context ============================================================ - -#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION - typedef node::async_context async_context; -#else - struct async_context {}; -#endif - // === AsyncResource =========================================================== class AsyncResource { @@ -1295,9 +1287,7 @@ class Utf8String { maybe_resource.IsEmpty() ? New() : maybe_resource.ToLocalChecked(); - node::async_context context = - node::EmitAsyncInit(isolate, resource, resource_name); - asyncContext = static_cast(context); + context = node::EmitAsyncInit(isolate, resource, resource_name); #endif } @@ -1310,18 +1300,14 @@ class Utf8String { : maybe_resource.ToLocalChecked(); v8::Local name_string = New(name).ToLocalChecked(); - node::async_context context = - node::EmitAsyncInit(isolate, resource, name_string); - asyncContext = static_cast(context); + context = node::EmitAsyncInit(isolate, resource, name_string); #endif } ~AsyncResource() { #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); + node::EmitAsyncDestroy(isolate, context); #endif } @@ -1334,8 +1320,7 @@ class Utf8String { return MakeCallback(target, func, argc, argv); #else return node::MakeCallback( - v8::Isolate::GetCurrent(), target, func, argc, argv, - static_cast(asyncContext)); + v8::Isolate::GetCurrent(), target, func, argc, argv, context); #endif } @@ -1348,8 +1333,7 @@ class Utf8String { return MakeCallback(target, symbol, argc, argv); #else return node::MakeCallback( - v8::Isolate::GetCurrent(), target, symbol, argc, argv, - static_cast(asyncContext)); + v8::Isolate::GetCurrent(), target, symbol, argc, argv, context); #endif } @@ -1362,13 +1346,15 @@ class Utf8String { return MakeCallback(target, method, argc, argv); #else return node::MakeCallback( - v8::Isolate::GetCurrent(), target, method, argc, argv, - static_cast(asyncContext)); + v8::Isolate::GetCurrent(), target, method, argc, argv, context); #endif } private: - async_context asyncContext; + NAN_DISALLOW_ASSIGN_COPY_MOVE(AsyncResource) +#if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION + node::async_context context; +#endif }; typedef void (*FreeCallback)(char *data, void *hint); From 8ec4423e34fd307d0ce8c0f139b9882fad10574f Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 09:56:33 -0800 Subject: [PATCH 4/9] Refactor Sleep into sleep.h --- Makefile | 1 + test/cpp/asyncresource.cpp | 4 ++-- test/cpp/asyncworker.cpp | 5 +---- test/cpp/callbackcontext.cpp | 4 ++-- test/cpp/sleep.h | 14 ++++++++++++++ 5 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 test/cpp/sleep.h diff --git a/Makefile b/Makefile index 9e8a2a3a..fc59a6d6 100644 --- a/Makefile +++ b/Makefile @@ -71,6 +71,7 @@ LINT_SOURCES = \ test/cpp/returnvalue.cpp \ test/cpp/setcallhandler.cpp \ test/cpp/settemplate.cpp \ + test/cpp/sleeph.h \ test/cpp/strings.cpp \ test/cpp/symbols.cpp \ test/cpp/threadlocal.cpp \ diff --git a/test/cpp/asyncresource.cpp b/test/cpp/asyncresource.cpp index 8777e661..fc9fc1e0 100644 --- a/test/cpp/asyncresource.cpp +++ b/test/cpp/asyncresource.cpp @@ -7,7 +7,7 @@ ********************************************************************/ #include -#include +#include "sleep.h" using namespace Nan; // NOLINT(build/namespaces) @@ -30,7 +30,7 @@ class DelayRequest : public AsyncResource { void Delay(uv_work_t* req) { DelayRequest *delay_request = static_cast(req->data); - sleep(delay_request->milliseconds / 1000); + Sleep(delay_request->milliseconds); } void AfterDelay(uv_work_t* req, int status) { diff --git a/test/cpp/asyncworker.cpp b/test/cpp/asyncworker.cpp index 36822d46..f8c86b03 100644 --- a/test/cpp/asyncworker.cpp +++ b/test/cpp/asyncworker.cpp @@ -6,10 +6,7 @@ * MIT License ********************************************************************/ -#ifndef _WIN32 -#include -#define Sleep(x) usleep((x)*1000) -#endif +#include "sleep.h" #include using namespace Nan; // NOLINT(build/namespaces) diff --git a/test/cpp/callbackcontext.cpp b/test/cpp/callbackcontext.cpp index 14cc8293..a627acb2 100644 --- a/test/cpp/callbackcontext.cpp +++ b/test/cpp/callbackcontext.cpp @@ -7,7 +7,7 @@ ********************************************************************/ #include -#include +#include "sleep.h" using namespace Nan; // NOLINT(build/namespaces) @@ -28,7 +28,7 @@ class DelayRequest : public AsyncResource { void Delay(uv_work_t* req) { DelayRequest *delay_request = static_cast(req->data); - sleep(delay_request->milliseconds / 1000); + Sleep(delay_request->milliseconds); } void AfterDelay(uv_work_t* req, int status) { diff --git a/test/cpp/sleep.h b/test/cpp/sleep.h new file mode 100644 index 00000000..b82c07a2 --- /dev/null +++ b/test/cpp/sleep.h @@ -0,0 +1,14 @@ +/********************************************************************* + * NAN - Native Abstractions for Node.js + * + * Copyright (c) 2017 NAN contributors + * + * MIT License + ********************************************************************/ + +#ifndef _WIN32 +#include +#define Sleep(x) usleep((x)*1000) +#endif + + From f150b27623e8a04d6b0ef583758d6405af01d77d Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 10:18:38 -0800 Subject: [PATCH 5/9] New Callback::Call functions should return MaybeLocal --- nan.h | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/nan.h b/nan.h index 54a8d598..8518e70c 100644 --- a/nan.h +++ b/nan.h @@ -1486,14 +1486,14 @@ class Callback { return this->Call(target, argc, argv); } - inline v8::Local operator()( + inline v8::MaybeLocal operator()( AsyncResource* resource , int argc = 0 , v8::Local argv[] = 0) const { return this->Call(argc, argv, resource); } - inline v8::Local operator()( + inline v8::MaybeLocal operator()( AsyncResource* resource , v8::Local target , int argc = 0 @@ -1553,14 +1553,13 @@ class Callback { #endif } - inline v8::Local + inline v8::MaybeLocal Call(v8::Local target , int argc , v8::Local argv[] , AsyncResource* resource) const { #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION v8::Isolate* isolate = v8::Isolate::GetCurrent(); - return Call_(isolate, target, argc, argv, resource); #elif NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION v8::Isolate *isolate = v8::Isolate::GetCurrent(); @@ -1570,16 +1569,11 @@ class Callback { #endif } - inline v8::Local + inline v8::MaybeLocal Call(int argc, v8::Local argv[], AsyncResource* resource) const { #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::EscapableHandleScope scope(isolate); - return scope.Escape(Call_(isolate - , isolate->GetCurrentContext()->Global() - , argc - , argv - , resource)); + return Call(isolate->GetCurrentContext()->Global(), argc, argv, resource); #elif NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION v8::Isolate *isolate = v8::Isolate::GetCurrent(); v8::EscapableHandleScope scope(isolate); @@ -1596,16 +1590,17 @@ class Callback { Persistent handle_; #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION - v8::Local Call_(v8::Isolate *isolate - , v8::Local target - , int argc - , v8::Local argv[] - , AsyncResource* resource) const { + v8::MaybeLocal Call_(v8::Isolate *isolate + , v8::Local target + , int argc + , v8::Local argv[] + , AsyncResource* resource) const { EscapableHandleScope scope; - v8::Local func = New(handle_); - return scope.Escape(resource->runInAsyncScope(target, func, argc, argv) - .ToLocalChecked()); + auto maybe = resource->runInAsyncScope(target, func, argc, argv); + v8::Local local; + if (!maybe.ToLocal(&local)) return v8::MaybeLocal(); + return scope.Escape(local); } #endif From 78712b16d691726848f85f44aa24945afb33bd06 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 11:32:09 -0800 Subject: [PATCH 6/9] fix compile problem with older node versions --- nan.h | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/nan.h b/nan.h index 8518e70c..66e941f1 100644 --- a/nan.h +++ b/nan.h @@ -1486,14 +1486,20 @@ class Callback { return this->Call(target, argc, argv); } - inline v8::MaybeLocal operator()( + inline v8::Local operator()( + int argc = 0 + , v8::Local argv[] = 0) const { + return this->Call(argc, argv); + } + + inline MaybeLocal operator()( AsyncResource* resource , int argc = 0 , v8::Local argv[] = 0) const { return this->Call(argc, argv, resource); } - inline v8::MaybeLocal operator()( + inline MaybeLocal operator()( AsyncResource* resource , v8::Local target , int argc = 0 @@ -1501,12 +1507,6 @@ class Callback { return this->Call(target, argc, argv, resource); } - inline v8::Local operator()( - int argc = 0 - , v8::Local argv[] = 0) const { - return this->Call(argc, argv); - } - // TODO(kkoopa): remove inline void SetFunction(const v8::Local &fn) { Reset(fn); @@ -1553,7 +1553,7 @@ class Callback { #endif } - inline v8::MaybeLocal + inline MaybeLocal Call(v8::Local target , int argc , v8::Local argv[] @@ -1569,7 +1569,7 @@ class Callback { #endif } - inline v8::MaybeLocal + inline MaybeLocal Call(int argc, v8::Local argv[], AsyncResource* resource) const { #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION v8::Isolate* isolate = v8::Isolate::GetCurrent(); @@ -1590,16 +1590,16 @@ class Callback { Persistent handle_; #if NODE_MODULE_VERSION >= NODE_8_0_MODULE_VERSION - v8::MaybeLocal Call_(v8::Isolate *isolate - , v8::Local target - , int argc - , v8::Local argv[] - , AsyncResource* resource) const { + MaybeLocal Call_(v8::Isolate *isolate + , v8::Local target + , int argc + , v8::Local argv[] + , AsyncResource* resource) const { EscapableHandleScope scope; v8::Local func = New(handle_); auto maybe = resource->runInAsyncScope(target, func, argc, argv); v8::Local local; - if (!maybe.ToLocal(&local)) return v8::MaybeLocal(); + if (!maybe.ToLocal(&local)) return MaybeLocal(); return scope.Escape(local); } #endif From 65e37fa991ad43d5a895bc44304a245c61122924 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 11:35:59 -0800 Subject: [PATCH 7/9] fix typo in Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index fc59a6d6..44e22c25 100644 --- a/Makefile +++ b/Makefile @@ -71,7 +71,7 @@ LINT_SOURCES = \ test/cpp/returnvalue.cpp \ test/cpp/setcallhandler.cpp \ test/cpp/settemplate.cpp \ - test/cpp/sleeph.h \ + test/cpp/sleep.h \ test/cpp/strings.cpp \ test/cpp/symbols.cpp \ test/cpp/threadlocal.cpp \ From 881cb4c88c6f0f2c57287e6b672dbb5a71414826 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 11:37:45 -0800 Subject: [PATCH 8/9] make docs consistent with the api again --- doc/callback.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/callback.md b/doc/callback.md index dc862068..60719352 100644 --- a/doc/callback.md +++ b/doc/callback.md @@ -22,14 +22,14 @@ class Callback { v8::Local operator*() const; - v8::Local operator()(AsyncResource* async_resource, - v8::Local target, - int argc = 0, - v8::Local argv[] = 0) const; + MaybeLocal operator()(AsyncResource* async_resource, + v8::Local target, + int argc = 0, + v8::Local argv[] = 0) const; - v8::Local operator()(AsyncResource* async_resource, - int argc = 0, - v8::Local argv[] = 0) const; + MaybeLocal operator()(AsyncResource* async_resource, + int argc = 0, + v8::Local argv[] = 0) const; void SetFunction(const v8::Local &fn); @@ -41,13 +41,13 @@ class Callback { void Reset(); - v8::Local Call(v8::Local target, + MaybeLocal Call(v8::Local target, int argc, v8::Local argv[], AsyncResource* async_resource) const; - v8::Local Call(int argc, - v8::Local argv[], - AsyncResource* async_resource) const; + MaybeLocal Call(int argc, + v8::Local argv[], + AsyncResource* async_resource) const; // Legacy versions. Use the versions that accept an async_resource instead // as they run the callback in the correct async context as specified by the From 36c3801c55302ea52900947f093775102a0d1b17 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 9 Feb 2018 13:09:17 -0800 Subject: [PATCH 9/9] address code review comments --- doc/asyncworker.md | 2 +- test/cpp/sleep.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/doc/asyncworker.md b/doc/asyncworker.md index 5cfc77c4..239de801 100644 --- a/doc/asyncworker.md +++ b/doc/asyncworker.md @@ -15,7 +15,7 @@ This class internally handles the details of creating an [`AsyncResource`][AsyncResource], and running the callback in the correct async context. To be able to identify the async resources created by this class in async-hooks, provide a `resource_name` to the constructor. It is recommended that the module name be used as a prefix to the `resource_name` to avoid -collisions in the names. For more details see [`AsyncResource`][AsyncResource] documentation. The `resource_name` needs to stay valid for the lifetime of worker instance. +collisions in the names. For more details see [`AsyncResource`][AsyncResource] documentation. The `resource_name` needs to stay valid for the lifetime of the worker instance. Definition: diff --git a/test/cpp/sleep.h b/test/cpp/sleep.h index b82c07a2..fe7946dc 100644 --- a/test/cpp/sleep.h +++ b/test/cpp/sleep.h @@ -10,5 +10,3 @@ #include #define Sleep(x) usleep((x)*1000) #endif - -