async-wrap: always call before and after hooks

[AndreasMadsen]

This patch allows the before and after hook to be called even if the
kCallInitHook flag is 0.

• test/parallel/test-async-wrap-no-init-hook.js did not pass before.
• test/parallel/test-async-wrap-init-hook.js did pass before. I just added it for the sake of completeness.

issue tracking: AndreasMadsen/trace#12

[bnoordhuis]

Style: at the very least, rename it to enabled_, make it private and only accessible through a getter and setter. The getter should be const.

That said, I believe AsyncHooks is exposed to JS land. In that case, enabled should be a new element in the fields_ array.

[AndreasMadsen]

I have exposed it though the fields_ array as you sugested. Is this something there should be tested? It looks like async-wrap have a different testing policy.

[bnoordhuis]

I brought it up mostly because it looked incongruous. I'll defer to @trevnorris here.

[bnoordhuis]

I suggest R=@trevnorris.

[trevnorris]

No C-style casts.

[trevnorris]

@AndreasMadsen Your implementation looks nice. Just left a few comments. :)

[AndreasMadsen]

@trevnorris I have now made the requested changes.

[AndreasMadsen]

I'm not completely sure about the kEnabled implementation. async_hooks_pre_function and async_hooks_post_function are also called from another location. But I didn't make any changes there, my best guess was that it isn't necessary.

[trevnorris]

@AndreasMadsen Actually, now that I think about it, this change is unsafe. If kCallInitHook is 0 when the constructor is run then the object hasn't gone through the proper instantiation in AsyncWrap::AsyncWrap() and the object may be incomplete due to whatever tooling is setup by module developers. Because the object context is inspect-able via this in before/after.

Do you have a use case for allowing callbacks to be run in before/after if the object hasn't gone through proper instantiation?

[AndreasMadsen]

Hmm, you will have to clarify a few things before I can answer that question.

If kCallInitHook is 0 when the constructor is run ...

what constructor are you referring too. The wrap objects there is inspectable vis this in the hooks functions?

incomplete due to whatever tooling ...

from what perspective is it incomplete. The module developers?

Do you have a use case for allowing callbacks to be run in before/after if the object hasn't gone through proper instantiation?

So if I understand correctly, you are asking about what the expected behaviour is in this case:

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']); // behaviour 1
  // or
  assert.deepEqual(order, ['callback']); // behaviour 2
});

asyncHooksObject[0 /* kCallInitHook */] = 1;

If I understand correctly, I don't have any meaningful use case for behaviour 1. I suppose you could make a module to debug what keeps a process temporarily hanging and to avoid delay by async_wrap it doesn't get fully enabled (kCallInitHook = 1) before the module user expects the process to exit. But why call setupHooks at all in the beginning of the process lifespan.

I any case it would prefer behaviour 1 as it is easy to filter unnecessary hook calls from userland. But missing some hook calls is not fixable from userland. behaviour 2 would also mean that kCallInitHook affects the before & after hooks and that would be wired.

[Fishrock123]

@trevnorris status?

[trevnorris]

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?

[AndreasMadsen]

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

[trevnorris]

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.

[AndreasMadsen]

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.

[AndreasMadsen]

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.

[trevnorris]

want to change the name to kEnableHooks?

[AndreasMadsen]

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

[AndreasMadsen]

I think @trevnorris fixed this in 05f4dff, so the only value this pull requests adds is:

• allows later disabling of async_wrap, though this is more of a side effect and isn't tested
• adds tests cases, which for sure is needed.

[Qard]

As far as I can tell, after #1614 most of this PR is no longer applicable.

I'm closing this, but feel free to re-open or make a new PR for the test cases.