Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Commit

Permalink
fix: propagate async context
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ofrobots committed Mar 19, 2018
1 parent 7648fc4 commit af79443
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 5 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
16 changes: 14 additions & 2 deletions src/binding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Value> 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;

Expand All @@ -245,15 +253,15 @@ 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)
const char* err = sass_context_get_error_json(ctx);
v8::Local<v8::Value> argv[] = {
Nan::New<v8::String>(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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 5 additions & 2 deletions src/callback_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class CallbackBridge {
virtual std::vector<v8::Local<v8::Value>> pre_process_args(std::vector<L>) const =0;

Nan::Callback* callback;
Nan::AsyncResource* async_resource;
bool is_sync;

uv_mutex_t cv_mutex;
Expand All @@ -66,6 +67,7 @@ CallbackBridge<T, L>::CallbackBridge(v8::Local<v8::Function> 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<v8::Function> func = CallbackBridge<T, L>::get_wrapper_constructor().ToLocalChecked();
Expand All @@ -82,6 +84,7 @@ CallbackBridge<T, L>::~CallbackBridge() {

if (!is_sync) {
uv_close((uv_handle_t*)this->async, &async_gone);
delete this->async_resource;
}
}

Expand All @@ -107,7 +110,7 @@ T CallbackBridge<T, L>::operator()(std::vector<void*> 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<v8::Value>())
);
} else {
/*
Expand Down Expand Up @@ -159,7 +162,7 @@ void CallbackBridge<T, L>::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);
Expand Down
3 changes: 3 additions & 0 deletions src/sass_context_wrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/sass_context_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ extern "C" {

// v8 and nan related
Nan::Persistent<v8::Object> result;
Nan::AsyncResource* async_resource;
Nan::Callback* error_callback;
Nan::Callback* success_callback;

Expand Down

0 comments on commit af79443

Please sign in to comment.