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.

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

AsyncWrap minor fixes #1614

Closed
wants to merge 4 commits into from

Conversation

trevnorris
Copy link
Contributor

Finally getting around to finishing the AsyncWrap implementation. Here are some mostly minor changes. One specifically is breaking. Instead of needing to set the object field to enable/disable callbacks there are now methods on the async_wrap import. e.g.

var async_wrap = process.binding('async_wrap');
async_wrap.setupHooks(asyncInit, asyncBefore, asyncAfter);

async_wrap.enable();
// do stuff
async_wrap.disable();

R=@bnoordhuis

Some calls to ReqWrap would get through the initial check and allow the
init callback to run, even though the callback had not been used on the
parent. Fix by explicitly checking if the parent has a queue.

Also change the name of the check, and internal field of AsyncHooks.
Other names were confusing.
@bnoordhuis
Copy link
Member

LGTM with a suggestion.

Allow the init callback to see the PROVIDER type easily by being able to
compare the flag with the list of providers on the exported async_wrap
object.
Setting flags using cryptic numeric object fields is confusing. Instead
use much simpler .enable()/.disable() calls on the async_wrap object.
It doesn't make sense to call before/after callbacks in init to the
parent because they'll be made anyway from MakeCallback. If information
does need to be propagated then it should be done automatically. Will
deal with this if the issue arrises in the future.
@trevnorris
Copy link
Contributor Author

@bnoordhuis thanks. addressed both comments.

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 4, 2015
@@ -62,6 +58,8 @@ static void Initialize(Handle<Object> target,
HandleScope scope(isolate);

NODE_SET_METHOD(target, "setupHooks", SetupHooks);
Copy link
Member

Choose a reason for hiding this comment

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

I would do the same for SetupHooks for consistency. That can wait for another PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might as well. had to run before I had a chance to land this.

@bnoordhuis
Copy link
Member

LGTM

trevnorris added a commit that referenced this pull request May 5, 2015
Some calls to ReqWrap would get through the initial check and allow the
init callback to run, even though the callback had not been used on the
parent. Fix by explicitly checking if the parent has a queue.

Also change the name of the check, and internal field of AsyncHooks.
Other names were confusing.

PR-URL: #1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
trevnorris added a commit that referenced this pull request May 5, 2015
Allow the init callback to see the PROVIDER type easily by being able to
compare the flag with the list of providers on the exported async_wrap
object.

PR-URL: #1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
trevnorris added a commit that referenced this pull request May 5, 2015
Setting flags using cryptic numeric object fields is confusing. Instead
use much simpler .enable()/.disable() calls on the async_wrap object.

PR-URL: #1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
trevnorris added a commit that referenced this pull request May 5, 2015
It doesn't make sense to call before/after callbacks in init to the
parent because they'll be made anyway from MakeCallback. If information
does need to be propagated then it should be done automatically. Will
deal with this if the issue arrises in the future.

PR-URL: #1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris
Copy link
Contributor Author

Thanks. Landed in 84bf609, 4b2c786, bd42ba0 and 7dde95a.

@trevnorris trevnorris closed this May 5, 2015
@trevnorris trevnorris deleted the async-wrap-minor-fixes branch May 5, 2015 02:09
@rvagg rvagg mentioned this pull request May 5, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
Some calls to ReqWrap would get through the initial check and allow the
init callback to run, even though the callback had not been used on the
parent. Fix by explicitly checking if the parent has a queue.

Also change the name of the check, and internal field of AsyncHooks.
Other names were confusing.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
Allow the init callback to see the PROVIDER type easily by being able to
compare the flag with the list of providers on the exported async_wrap
object.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
Setting flags using cryptic numeric object fields is confusing. Instead
use much simpler .enable()/.disable() calls on the async_wrap object.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
It doesn't make sense to call before/after callbacks in init to the
parent because they'll be made anyway from MakeCallback. If information
does need to be propagated then it should be done automatically. Will
deal with this if the issue arrises in the future.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants