Skip to content

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

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

Closed
AndreasMadsen 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 1 commit
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
Prev Previous commit
Next Next commit
async-wrap: expose enabled flag to JS
  • Loading branch information
@AndreasMadsen
AndreasMadsen committed Jan 30, 2015
commit af0fff2293f6550b1161b7faa064ee5c56c2104b
4 changes: 3 additions & 1 deletion src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,16 @@ 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

@trevnorris trevnorris Jan 30, 2015

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

@trevnorris trevnorris Jan 30, 2015

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

@AndreasMadsen AndreasMadsen Jan 30, 2015

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->enabled = true;
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>());
Expand Down
11 changes: 9 additions & 2 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ inline v8::Isolate* Environment::IsolateData::isolate() const {
}

inline Environment::AsyncHooks::AsyncHooks() {
enabled = false;
for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0;
}

Expand All @@ -71,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] = (uint32_t)enabled;
Copy link
Contributor

@trevnorris trevnorris Jan 30, 2015

Choose a reason for hiding this comment

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

No C-style casts.

}

inline Environment::DomainFlag::DomainFlag() {
for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0;
}
Expand Down Expand Up @@ -217,7 +224,7 @@ inline v8::Isolate* Environment::isolate() const {

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()->enabled;
return const_cast<Environment*>(this)->async_hooks()->is_enabled();
}

inline bool Environment::call_async_init_hook() const {
Expand Down
6 changes: 5 additions & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ class Environment {
inline uint32_t* fields();
inline int fields_count() const;
inline bool call_init_hook();
bool enabled;
inline bool is_enabled() const;
inline void set_enabled(bool enabled);

private:
friend class Environment; // So we can call the constructor.
Expand All @@ -264,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

@trevnorris trevnorris Mar 13, 2015

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

@AndreasMadsen AndreasMadsen Mar 13, 2015

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

@trevnorris trevnorris Mar 13, 2015

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

@AndreasMadsen AndreasMadsen Mar 13, 2015

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

@AndreasMadsen AndreasMadsen Mar 27, 2015

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

@trevnorris trevnorris Mar 27, 2015

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

@AndreasMadsen AndreasMadsen Mar 27, 2015

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