From e46be3b954d55c0861fd068f29c974b6ab3f06de 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 This is an alternative to some of the fixes in [1]. 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. [1]: https://github.com/sass/node-sass/pull/2295 --- src/binding.cpp | 17 ++++++++++++++--- src/callback_bridge.h | 8 +++++--- src/sass_context_wrapper.cpp | 3 +++ src/sass_context_wrapper.h | 1 + 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/binding.cpp b/src/binding.cpp index de0a5234a..abf298fcf 100644 --- a/src/binding.cpp +++ b/src/binding.cpp @@ -227,9 +227,16 @@ 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; - Nan::AsyncResource async("sass:MakeCallback"); Nan::TryCatch try_catch; sass_context_wrapper* ctx_w = static_cast(req->data); @@ -246,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, &async); + PerformCall(ctx_w, ctx_w->success_callback, 0, 0); } else if (ctx_w->error_callback) { // if error, do callback(error) @@ -254,7 +261,7 @@ void MakeCallback(uv_work_t* req) { v8::Local argv[] = { Nan::New(err).ToLocalChecked() }; - ctx_w->error_callback->Call(1, argv, &async); + PerformCall(ctx_w, ctx_w->error_callback, 1, argv); } if (try_catch.HasCaught()) { Nan::FatalException(try_catch); @@ -270,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:render"); + 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); @@ -304,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:render_file"); + 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 68ecaa809..25f62e140 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( - Nan::Call(*callback, argv_v8.size(), &argv_v8[0]).ToLocalChecked() + Nan::Call(*this->callback, argv_v8.size(), &argv_v8[0]).ToLocalChecked() ); } else { /* @@ -151,7 +154,6 @@ void CallbackBridge::dispatched_async_uv_callback(uv_async_t *req) { * post_process_args(). */ Nan::HandleScope scope; - Nan::AsyncResource async("sass:CallbackBridge"); Nan::TryCatch try_catch; std::vector> argv_v8 = bridge->pre_process_args(bridge->argv); @@ -160,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], &async); + 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;