Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Asyncwrap more #5756

Merged
merged 5 commits into from
Mar 28, 2016
Merged

Asyncwrap more #5756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
41f333e
src,http_parser: remove KickNextTick call
trevnorris Mar 11, 2016
2dadd89
src: reword command and add ternary
trevnorris Mar 23, 2016
f9938b6
async_wrap: setupHooks now accepts object
trevnorris Feb 22, 2016
20337ad
async_wrap: notify post if intercepted exception
trevnorris Feb 23, 2016
a17200b
async_wrap: don't abort on callback exception
trevnorris Mar 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/async-wrap-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,15 @@ inline AsyncWrap::AsyncWrap(Environment* env,
argv[3] = parent->object();
}

v8::TryCatch try_catch(env->isolate());

v8::MaybeLocal<v8::Value> ret =
init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv);

if (ret.IsEmpty())
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
}

bits_ |= 1; // ran_init_callback() is true now.
}
Expand All @@ -69,10 +73,13 @@ inline AsyncWrap::~AsyncWrap() {
if (!fn.IsEmpty()) {
v8::HandleScope scope(env()->isolate());
v8::Local<v8::Value> uid = v8::Integer::New(env()->isolate(), get_uid());
v8::TryCatch try_catch(env()->isolate());
v8::MaybeLocal<v8::Value> ret =
fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid);
if (ret.IsEmpty())
FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw");
if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
}
}
}

Expand Down
65 changes: 49 additions & 16 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "v8-profiler.h"

using v8::Array;
using v8::Boolean;
using v8::Context;
using v8::Function;
using v8::FunctionCallbackInfo;
Expand All @@ -17,6 +18,7 @@ using v8::HeapProfiler;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
using v8::Object;
using v8::RetainedObjectInfo;
using v8::TryCatch;
Expand Down Expand Up @@ -121,18 +123,35 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {

if (env->async_hooks()->callbacks_enabled())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for requiring callers to explicitly disable callbacks, vs implicitly disabling them & restoring them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing context. Users have to explicitly run async_wrap.enable() before callbacks will be called. This case is to handle:

const async_wrap = require('async_wrap');
async_wrap.setupHooks({ ...callbacks... });
async_wrap.enable();
async_wrap.setupHooks({ ...different callbacks... });

To prevent callbacks from being pulled out from underneath the user while operations may be in flight.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, question is why is it the responsibility of the setupHooks() caller to enable()/disable()? i.e., why can't this be done implicitly by setupHooks()?

Same question follows when the handlers are invoked. We know that if an init() callback triggers an async operation, it risks infinite recursion, so why not implicitly disable async callbacks before invoking init()?

return env->ThrowError("hooks should not be set while also enabled");

if (!args[0]->IsFunction())
if (!args[0]->IsObject())
return env->ThrowTypeError("first argument must be an object");

Local<Object> fn_obj = args[0].As<Object>();

Local<Value> init_v = fn_obj->Get(
env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "init")).ToLocalChecked();
Local<Value> pre_v = fn_obj->Get(
env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "pre")).ToLocalChecked();
Local<Value> post_v = fn_obj->Get(
env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "post")).ToLocalChecked();
Local<Value> destroy_v = fn_obj->Get(
env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "destroy")).ToLocalChecked();

if (!init_v->IsFunction())
return env->ThrowTypeError("init callback must be a function");

env->set_async_hooks_init_function(args[0].As<Function>());
env->set_async_hooks_init_function(init_v.As<Function>());

if (args[1]->IsFunction())
env->set_async_hooks_pre_function(args[1].As<Function>());
if (args[2]->IsFunction())
env->set_async_hooks_post_function(args[2].As<Function>());
if (args[3]->IsFunction())
env->set_async_hooks_destroy_function(args[3].As<Function>());
if (pre_v->IsFunction())
env->set_async_hooks_pre_function(pre_v.As<Function>());
if (post_v->IsFunction())
env->set_async_hooks_post_function(post_v.As<Function>());
if (destroy_v->IsFunction())
env->set_async_hooks_destroy_function(destroy_v.As<Function>());
}


Expand Down Expand Up @@ -181,7 +200,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Function> post_fn = env()->async_hooks_post_function();
Local<Value> uid = Integer::New(env()->isolate(), get_uid());
Local<Object> context = object();
Local<Object> process = env()->process_object();
Local<Object> domain;
bool has_domain = false;

Expand All @@ -208,15 +226,28 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}

if (ran_init_callback() && !pre_fn.IsEmpty()) {
if (pre_fn->Call(context, 1, &uid).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
TryCatch try_catch(env()->isolate());
MaybeLocal<Value> ar = pre_fn->Call(env()->context(), context, 1, &uid);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
return Local<Value>();
}
}

Local<Value> ret = cb->Call(context, argc, argv);

if (ran_init_callback() && !post_fn.IsEmpty()) {
if (post_fn->Call(context, 1, &uid).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
Local<Value> vals[] = { uid, did_throw };
TryCatch try_catch(env()->isolate());
MaybeLocal<Value> ar =
post_fn->Call(env()->context(), context, ARRAY_SIZE(vals), vals);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env());
FatalException(env()->isolate(), try_catch);
return Local<Value>();
}
}

if (ret.IsEmpty()) {
Expand All @@ -233,16 +264,18 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
}

Environment::TickInfo* tick_info = env()->tick_info();

if (callback_scope.in_makecallback()) {
return ret;
}

Environment::TickInfo* tick_info = env()->tick_info();

if (tick_info->length() == 0) {
env()->isolate()->RunMicrotasks();
}

Local<Object> process = env()->process_object();

if (tick_info->length() == 0) {
tick_info->set_index(0);
return ret;
Expand Down
23 changes: 0 additions & 23 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,27 +64,4 @@ void Environment::PrintSyncTrace() const {
fflush(stderr);
}


bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
TickInfo* info = tick_info();

if (scope->in_makecallback()) {
return true;
}

if (info->length() == 0) {
isolate()->RunMicrotasks();
}

if (info->length() == 0) {
info->set_index(0);
return true;
}

Local<Value> ret =
tick_callback_function()->Call(process_object(), 0, nullptr);

return !ret.IsEmpty();
}

} // namespace node
2 changes: 0 additions & 2 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,6 @@ class Environment {

inline int64_t get_async_wrap_uid();

bool KickNextTick(AsyncCallbackScope* scope);

inline uint32_t* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(uint32_t* pointer);

Expand Down
70 changes: 60 additions & 10 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,23 +1193,38 @@ Local<Value> MakeCallback(Environment* env,
}

if (ran_init_callback && !pre_fn.IsEmpty()) {
if (pre_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "pre hook threw");
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar = pre_fn->Call(env->context(), object, 0, nullptr);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
return Local<Value>();
}
}

Local<Value> ret = callback->Call(recv, argc, argv);

if (ran_init_callback && !post_fn.IsEmpty()) {
if (post_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "post hook threw");
Local<Value> did_throw = Boolean::New(env->isolate(), ret.IsEmpty());
// Currently there's no way to retrieve an uid from node::MakeCallback().
// This needs to be fixed.
Local<Value> vals[] =
{ Undefined(env->isolate()).As<Value>(), did_throw };
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar =
post_fn->Call(env->context(), object, ARRAY_SIZE(vals), vals);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
return Local<Value>();
}
}

if (ret.IsEmpty()) {
if (callback_scope.in_makecallback())
return ret;
// NOTE: Undefined() is returned here for backwards compatibility.
else
return Undefined(env->isolate());
// NOTE: For backwards compatibility with public API we return Undefined()
// if the top level call threw.
return callback_scope.in_makecallback() ?
ret : Undefined(env->isolate()).As<Value>();
}

if (has_domain) {
Expand All @@ -1222,7 +1237,23 @@ Local<Value> MakeCallback(Environment* env,
}
}

if (!env->KickNextTick(&callback_scope)) {
if (callback_scope.in_makecallback()) {
return ret;
}

Environment::TickInfo* tick_info = env->tick_info();

if (tick_info->length() == 0) {
env->isolate()->RunMicrotasks();
}

Local<Object> process = env->process_object();

if (tick_info->length() == 0) {
tick_info->set_index(0);
}

if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
return Undefined(env->isolate());
}

Expand Down Expand Up @@ -2384,6 +2415,25 @@ void OnMessage(Local<Message> message, Local<Value> error) {
}


void ClearFatalExceptionHandlers(Environment* env) {
Local<Object> process = env->process_object();
Local<Value> events =
process->Get(env->context(), env->events_string()).ToLocalChecked();

if (events->IsObject()) {
events.As<Object>()->Set(
env->context(),
OneByteString(env->isolate(), "uncaughtException"),
Undefined(env->isolate())).FromJust();
}

process->Set(
env->context(),
env->domain_string(),
Undefined(env->isolate())).FromJust();
}


static void Binding(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down
4 changes: 0 additions & 4 deletions src/node_http_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,8 +587,6 @@ class Parser : public AsyncWrap {
if (!cb->IsFunction())
return;

Environment::AsyncCallbackScope callback_scope(parser->env());

// Hooks for GetCurrentBuffer
parser->current_buffer_len_ = nread;
parser->current_buffer_data_ = buf->base;
Expand All @@ -597,8 +595,6 @@ class Parser : public AsyncWrap {

parser->current_buffer_len_ = 0;
parser->current_buffer_data_ = nullptr;

parser->env()->KickNextTick(&callback_scope);
}


Expand Down
5 changes: 5 additions & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
Environment* env_;
};

// Clear any domain and/or uncaughtException handlers to force the error's
// propagation and shutdown the process. Use this to force the process to exit
// by clearing all callbacks that could handle the error.
void ClearFatalExceptionHandlers(Environment* env);

enum NodeInstanceType { MAIN, WORKER };

class NodeInstanceData {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-async-wrap-check-providers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function init(id, provider) {

function noop() { }

async_wrap.setupHooks(init, noop, noop);
async_wrap.setupHooks({ init });

async_wrap.enable();

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-async-wrap-disabled-propagate-parent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function init(uid, type, parentUid, parentHandle) {

function noop() { }

async_wrap.setupHooks(init, noop, noop);
async_wrap.setupHooks({ init });
async_wrap.enable();

server = net.createServer(function(c) {
Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-async-wrap-post-did-throw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';

require('../common');
const assert = require('assert');
const async_wrap = process.binding('async_wrap');
var asyncThrows = 0;
var uncaughtExceptionCount = 0;

process.on('uncaughtException', (e) => {
assert.equal(e.message, 'oh noes!', 'error messages do not match');
});

process.on('exit', () => {
process.removeAllListeners('uncaughtException');
assert.equal(uncaughtExceptionCount, 1);
assert.equal(uncaughtExceptionCount, asyncThrows);
});

function init() { }
function post(id, threw) {
if (threw)
uncaughtExceptionCount++;
}

async_wrap.setupHooks({ init, post });
async_wrap.enable();

// Timers still aren't supported, so use crypto API.
// It's also important that the callback not happen in a nextTick, like many
// error events in core.
require('crypto').randomBytes(0, () => {
asyncThrows++;
throw new Error('oh noes!');
});
2 changes: 1 addition & 1 deletion test/parallel/test-async-wrap-propagate-parent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function init(uid, type, parentUid, parentHandle) {

function noop() { }

async_wrap.setupHooks(init, noop, noop);
async_wrap.setupHooks({ init });
async_wrap.enable();

server = net.createServer(function(c) {
Expand Down
Loading