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

AndreasMadsen · 2015-01-30T13:53:27Z

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

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

bnoordhuis · 2015-01-30T14:11:41Z

src/env.h

@@ -255,6 +255,7 @@ class Environment {
    inline uint32_t* fields();
    inline int fields_count() const;
    inline bool call_init_hook();
+    bool enabled;


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.

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.

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

bnoordhuis · 2015-01-30T14:14:10Z

I suggest R=@trevnorris.

trevnorris · 2015-01-30T20:31:04Z

src/env-inl.h

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


No C-style casts.

trevnorris · 2015-01-30T20:37:32Z

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

AndreasMadsen · 2015-01-30T21:02:40Z

@trevnorris I have now made the requested changes.

AndreasMadsen · 2015-01-30T21:05:55Z

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 · 2015-01-30T22:03:27Z

@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 · 2015-01-30T22:49:54Z

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 · 2015-03-13T17:47:06Z

@trevnorris status?

trevnorris · 2015-03-13T17:58:33Z

src/env.h

@@ -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,


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?

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

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.

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.

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.

want to change the name to kEnableHooks?

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

AndreasMadsen · 2015-03-13T18:05:57Z

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 · 2015-06-25T00:06:03Z

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.

async-wrap: always call before and after hooks

c3ee11e

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

bnoordhuis reviewed Jan 30, 2015
View reviewed changes

async-wrap: expose enabled flag to JS

af0fff2

trevnorris reviewed Jan 30, 2015
View reviewed changes

AndreasMadsen added 2 commits January 30, 2015 21:58

async-wrap: use c++ style casting

b62788b

async-wrap: add enabled (disabled) test

d44132c

trevnorris self-assigned this Feb 5, 2015

trevnorris reviewed Mar 13, 2015
View reviewed changes

brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 7, 2015

chrisdickinson added the land-on-master label Apr 17, 2015

Qard closed this Jun 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AndreasMadsen commented Jan 30, 2015

bnoordhuis Jan 30, 2015

AndreasMadsen Jan 30, 2015

bnoordhuis Jan 30, 2015

bnoordhuis commented Jan 30, 2015

trevnorris Jan 30, 2015

trevnorris commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

trevnorris commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

Fishrock123 commented Mar 13, 2015

trevnorris Mar 13, 2015

AndreasMadsen Mar 13, 2015

trevnorris Mar 13, 2015

AndreasMadsen Mar 13, 2015

AndreasMadsen Mar 27, 2015

trevnorris Mar 27, 2015

AndreasMadsen Mar 27, 2015

AndreasMadsen commented Mar 13, 2015

Qard commented Jun 25, 2015

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

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

Conversation

AndreasMadsen commented Jan 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 30, 2015

Choose a reason for hiding this comment

trevnorris commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

trevnorris commented Jan 30, 2015

AndreasMadsen commented Jan 30, 2015

Fishrock123 commented Mar 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Mar 13, 2015

Qard commented Jun 25, 2015