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

async-wrap: always call before and after hooks #665

Closed
wants to merge 4 commits into from
Closed

async-wrap: always call before and after hooks #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c3ee11e
async-wrap: always call before and after hooks
AndreasMadsen Jan 30, 2015
af0fff2
async-wrap: expose enabled flag to JS
AndreasMadsen Jan 30, 2015
b62788b
async-wrap: use c++ style casting
AndreasMadsen Jan 30, 2015
d44132c
async-wrap: add enabled (disabled) test
AndreasMadsen Jan 30, 2015
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
14 changes: 8 additions & 6 deletions src/async-wrap-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ inline AsyncWrap::AsyncWrap(Environment* env,
: BaseObject(env, object),
has_async_queue_(false),
provider_type_(provider) {
// Check user controlled flag to see if the init callback should run.
if (!env->call_async_init_hook())
// Check user controlled flag to see if the async hooks should be called.
if (!env->use_async_hook())
return;

// TODO(trevnorris): Until it's verified all passed object's are not weak,
Expand All @@ -42,11 +42,13 @@ inline AsyncWrap::AsyncWrap(Environment* env,
FatalError("node::AsyncWrap::AsyncWrap", "parent pre hook threw");
}

env->async_hooks_init_function()->Call(object, 0, nullptr);

if (try_catch.HasCaught())
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
// Check user controlled flag to see if the init callback should run.
if (env->call_async_init_hook()) {
env->async_hooks_init_function()->Call(object, 0, nullptr);

if (try_catch.HasCaught())
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
}
has_async_queue_ = true;

if (parent != nullptr) {
Expand Down
4 changes: 4 additions & 0 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,17 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
// non-zero to have those callbacks called. This only affects the init
// callback. If the init callback was called, then the pre/post callbacks
// will automatically be called.
// - kEnabled (1): Tells the AsyncWrap constructor whether it should
// make any calls to the JS hook functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a test added to check kEnabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you distinguish what "JS hook functions" this means? There are 4 of them, and this doesn't affect them all.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 3 of them (init, pre and post), right? This should affect all of them.

I will add a test case then. I was just unsure about the test policy for async wrap.

Local<Object> async_hooks_obj = args[0].As<Object>();
Environment::AsyncHooks* async_hooks = env->async_hooks();
async_hooks_obj->SetIndexedPropertiesToExternalArrayData(
async_hooks->fields(),
kExternalUint32Array,
async_hooks->fields_count());

async_hooks->set_enabled(true);

env->set_async_hooks_init_function(args[1].As<Function>());
env->set_async_hooks_pre_function(args[2].As<Function>());
env->set_async_hooks_post_function(args[3].As<Function>());
Expand Down
13 changes: 13 additions & 0 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ inline bool Environment::AsyncHooks::call_init_hook() {
return fields_[kCallInitHook] != 0;
}

inline bool Environment::AsyncHooks::is_enabled() const {
return fields_[kEnabled] != 0;
}

inline void Environment::AsyncHooks::set_enabled(bool enabled) {
fields_[kEnabled] = static_cast<uint32_t>(enabled);
}

inline Environment::DomainFlag::DomainFlag() {
for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0;
}
Expand Down Expand Up @@ -214,6 +222,11 @@ inline v8::Isolate* Environment::isolate() const {
return isolate_;
}

inline bool Environment::use_async_hook() const {
// The const_cast is okay, it doesn't violate conceptual const-ness.
return const_cast<Environment*>(this)->async_hooks()->is_enabled();
}

inline bool Environment::call_async_init_hook() const {
// The const_cast is okay, it doesn't violate conceptual const-ness.
return const_cast<Environment*>(this)->async_hooks()->call_init_hook();
Expand Down
6 changes: 6 additions & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ class Environment {
inline uint32_t* fields();
inline int fields_count() const;
inline bool call_init_hook();
inline bool is_enabled() const;
inline void set_enabled(bool enabled);

private:
friend class Environment; // So we can call the constructor.
Expand All @@ -263,6 +265,9 @@ class Environment {
enum Fields {
// Set this to not zero if the init hook should be called.
kCallInitHook,
// Set this to not zero if async wrap should be enabled. This is
// automatically set when SetupHooks is called.
kEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the field is confusing. kCallInitHook is what "enables" init/before/after to be called. This field is an override that forces before/after to always be called regardless of whether init was called, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. kEnable is what "enables" init / before / after to be called. But init is only called if kCallInitHook is true.

EDIT: it was confusing before because kCallInitHook "enabled" init / before / after

Copy link
Contributor

Choose a reason for hiding this comment

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

I know you know this, but to reiterate. The point was that before/after don't run unless init runs, so enabling init implies enabling the others. I can see your confusion though, reading though the code the implicit constraint on what callbacks are called isn't apparent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know you know this, but to reiterate. The point was that before/after don't run unless init runs, so enabling init implies enabling the others.

Thanks for reiterating, I actually didn't realize that. The comment in (https://github.com/iojs/io.js/blob/v1.x/src/async-wrap.cc#L39) is rather confusing then:

This only affects the init callback.

I realize now that it also says:

If the init callback was called, then the pre/post callbacks will automatically be called.

which should be read as "if and only if". Sometimes I think my math education have kills all implicit human understanding :)

In any case I find it a bit confusing, the kCallInitHook should really just be called kEnable then. Or is it because before and after callbacks can still be called after kEnable is called?

I would propose having a kEnable which enables (1) or short circuit (0) the async_wrap stuff. Such that some or no callbacks are called.

I'm not sure how much of a performance hit a no-opt call is these days, but I guess having kCallInitHook, kCallBeforeHook and kCallAfterHook flags would make sense. The flags are such that the respective callback is only called the flag is set. For instance I don't think there is any reason for me to have an after hook in trace (L80)

The logic behind preventing the before and after hook calls if init wasn't called, is then removed. I don't think this is particularly useful, it seams like it is user protection and it is easy to implement in userland.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I have come to the conclusion that both behaviors are reasonable. However the kCallInitHook name is very confusion as it suggests it only effects the init hook, while really it affects all hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

want to change the name to kEnableHooks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I will refactor this PR to do that and redo the tests.

kFieldsCount
};

Expand Down Expand Up @@ -358,6 +363,7 @@ class Environment {

inline v8::Isolate* isolate() const;
inline uv_loop_t* event_loop() const;
inline bool use_async_hook() const;
inline bool call_async_init_hook() const;
inline bool in_domain() const;
inline uint32_t watched_providers() const;
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-async-wrap-disabled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
var common = require('../common');
var assert = require('assert');

var asyncWrap = process.binding('async_wrap');

var asyncHooksObject = {};
var kEnabled = 1;
asyncWrap.setupHooks(asyncHooksObject, init, before, after);
asyncHooksObject[kEnabled] = 0;

var order = [];

function init() {
order.push('init ' + this.constructor.name);
}

function before() {
order.push('before ' + this.constructor.name);
}

function after() {
order.push('after ' + this.constructor.name);
}

setTimeout(function () {
order.push('callback');
});

process.once('exit', function () {
assert.deepEqual(order, [
'callback'
]);
});
33 changes: 33 additions & 0 deletions test/parallel/test-async-wrap-init-hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
var common = require('../common');
var assert = require('assert');

var asyncWrap = process.binding('async_wrap');

var asyncHooksObject = {};
var kCallInitHook = 0;
asyncWrap.setupHooks(asyncHooksObject, init, before, after);
asyncHooksObject[kCallInitHook] = 1;

var order = [];

function init() {
order.push('init ' + this.constructor.name);
}

function before() {
order.push('before ' + this.constructor.name);
}

function after() {
order.push('after ' + this.constructor.name);
}

setTimeout(function () {
order.push('callback');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for a future note, this may be spotty because there's incomplete timer support. Because only a single TimerWrap is instantiated for many timers. Still need to implement the JS side for proper full support.


process.once('exit', function () {
assert.deepEqual(order, [
'init Timer', 'before Timer', 'callback', 'after Timer'
]);
});
31 changes: 31 additions & 0 deletions test/parallel/test-async-wrap-no-init-hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
var common = require('../common');
var assert = require('assert');

var asyncWrap = process.binding('async_wrap');

var asyncHooksObject = {};
asyncWrap.setupHooks(asyncHooksObject, init, before, after);

var order = [];

function init() {
order.push('init ' + this.constructor.name);
}

function before() {
order.push('before ' + this.constructor.name);
}

function after() {
order.push('after ' + this.constructor.name);
}

setTimeout(function () {
order.push('callback');
});

process.once('exit', function () {
assert.deepEqual(order, [
'before Timer', 'callback', 'after Timer'
]);
});