From af79443c4ef12456ff68a00a1b4d86cfffc6c4f7 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Wed, 14 Mar 2018 19:07:41 -0700 Subject: [PATCH] fix: propagate async context Starting with Nan 2.9.0, we have the ability to propagate async context across async hops. Certain variants of Nan::Callback::Call are now deprecated to encourage use of the context presevering alternatives. Certain variants of Node's MakeCallback that were used internally are going to be deprecated in Node 10. Summary is that one should use Nan::Call for sync calls, and Nan::Callback::Call for async. The latter expects an async resource corresponding to the async operation to be provided at the call time. This patch fixes things up so that 1) node-sass isn't using any deprecated APIs, and 2) properly propagates async context for async callbacks by creating async resources in the appropriate places. --- package.json | 2 +- src/binding.cpp | 16 ++++++++++++++-- src/callback_bridge.h | 7 +++++-- src/sass_context_wrapper.cpp | 3 +++ src/sass_context_wrapper.h | 1 + 5 files changed, 24 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 9d177e3cc..ac166b6a5 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "lodash.mergewith": "^4.6.0", "meow": "^3.7.0", "mkdirp": "^0.5.1", - "nan": "^2.9.2", + "nan": "^2.10.0", "node-gyp": "^3.3.1", "npmlog": "^4.0.0", "request": "~2.79.0", diff --git a/src/binding.cpp b/src/binding.cpp index 4f2cfdaac..b87be2636 100644 --- a/src/binding.cpp +++ b/src/binding.cpp @@ -227,6 +227,14 @@ int GetResult(sass_context_wrapper* ctx_w, Sass_Context* ctx, bool is_sync = fal return status; } +void PerformCall(sass_context_wrapper* ctx_w, Nan::Callback* callback, int argc, v8::Local argv[]) { + if (ctx_w->is_sync) { + Nan::Call(*callback, argc, argv); + } else { + callback->Call(argc, argv, ctx_w->async_resource); + } +} + void MakeCallback(uv_work_t* req) { Nan::HandleScope scope; @@ -245,7 +253,7 @@ void MakeCallback(uv_work_t* req) { if (status == 0 && ctx_w->success_callback) { // if no error, do callback(null, result) - ctx_w->success_callback->Call(0, 0); + PerformCall(ctx_w, ctx_w->success_callback, 0, 0); } else if (ctx_w->error_callback) { // if error, do callback(error) @@ -253,7 +261,7 @@ void MakeCallback(uv_work_t* req) { v8::Local argv[] = { Nan::New(err).ToLocalChecked() }; - ctx_w->error_callback->Call(1, argv); + PerformCall(ctx_w, ctx_w->error_callback, 1, argv); } if (try_catch.HasCaught()) { Nan::FatalException(try_catch); @@ -269,6 +277,8 @@ NAN_METHOD(render) { struct Sass_Data_Context* dctx = sass_make_data_context(source_string); sass_context_wrapper* ctx_w = sass_make_context_wrapper(); + ctx_w->async_resource = new Nan::AsyncResource("node-sass:sass_context_wrapper"); + if (ExtractOptions(options, dctx, ctx_w, false, false) >= 0) { int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback); @@ -303,6 +313,8 @@ NAN_METHOD(render_file) { struct Sass_File_Context* fctx = sass_make_file_context(input_path); sass_context_wrapper* ctx_w = sass_make_context_wrapper(); + ctx_w->async_resource = new Nan::AsyncResource("node-sass:sass_context_wrapper"); + if (ExtractOptions(options, fctx, ctx_w, true, false) >= 0) { int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback); diff --git a/src/callback_bridge.h b/src/callback_bridge.h index 918cf8930..6dbd52aca 100644 --- a/src/callback_bridge.h +++ b/src/callback_bridge.h @@ -40,6 +40,7 @@ class CallbackBridge { virtual std::vector> pre_process_args(std::vector) const =0; Nan::Callback* callback; + Nan::AsyncResource* async_resource; bool is_sync; uv_mutex_t cv_mutex; @@ -66,6 +67,7 @@ CallbackBridge::CallbackBridge(v8::Local callback, bool is_s this->async = new uv_async_t; this->async->data = (void*) this; uv_async_init(uv_default_loop(), this->async, (uv_async_cb) dispatched_async_uv_callback); + this->async_resource = new Nan::AsyncResource("node-sass:CallbackBridge"); } v8::Local func = CallbackBridge::get_wrapper_constructor().ToLocalChecked(); @@ -82,6 +84,7 @@ CallbackBridge::~CallbackBridge() { if (!is_sync) { uv_close((uv_handle_t*)this->async, &async_gone); + delete this->async_resource; } } @@ -107,7 +110,7 @@ T CallbackBridge::operator()(std::vector argv) { argv_v8.push_back(Nan::New(wrapper)); return this->post_process_return_value( - this->callback->Call(argv_v8.size(), &argv_v8[0]) + Nan::Call(*this->callback, argv_v8.size(), &argv_v8[0]).FromMaybe(v8::Local()) ); } else { /* @@ -159,7 +162,7 @@ void CallbackBridge::dispatched_async_uv_callback(uv_async_t *req) { } argv_v8.push_back(Nan::New(bridge->wrapper)); - bridge->callback->Call(argv_v8.size(), &argv_v8[0]); + bridge->callback->Call(argv_v8.size(), &argv_v8[0], bridge->async_resource); if (try_catch.HasCaught()) { Nan::FatalException(try_catch); diff --git a/src/sass_context_wrapper.cpp b/src/sass_context_wrapper.cpp index a3195c520..aa25c79b0 100644 --- a/src/sass_context_wrapper.cpp +++ b/src/sass_context_wrapper.cpp @@ -33,6 +33,9 @@ extern "C" { else if (ctx_w->fctx) { sass_delete_file_context(ctx_w->fctx); } + if (ctx_w->async_resource) { + delete ctx_w->async_resource; + } delete ctx_w->error_callback; delete ctx_w->success_callback; diff --git a/src/sass_context_wrapper.h b/src/sass_context_wrapper.h index 0da650139..4aa35684a 100644 --- a/src/sass_context_wrapper.h +++ b/src/sass_context_wrapper.h @@ -39,6 +39,7 @@ extern "C" { // v8 and nan related Nan::Persistent result; + Nan::AsyncResource* async_resource; Nan::Callback* error_callback; Nan::Callback* success_callback;