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

Jump to bottom

AsyncWrap minor fixes #1614

Closed
wants to merge 4 commits into from
Closed

AsyncWrap minor fixes #1614

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

@trevnorris
async-wrap: don't call init callback unnecessarily
3f51590 
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.

@trevnorris
async-wrap: pass PROVIDER as first arg to init
cc0385e 
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.
@trevnorris
async-wrap: set flags using functions
7740d5e 
Setting flags using cryptic numeric object fields is confusing. Instead
use much simpler .enable()/.disable() calls on the async_wrap object.
@trevnorris
async-wrap: remove before/after calls in init
eed38b3 
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
bnoordhuis
bnoordhuis reviewed May 4, 2015
View reviewed changes
src/async-wrap.cc
@@ -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
@trevnorris
async-wrap: don't call init callback unnecessarily
84bf609 
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
@trevnorris
async-wrap: pass PROVIDER as first arg to init
4b2c786 
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
@trevnorris
async-wrap: set flags using functions
bd42ba0 
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
@trevnorris
async-wrap: remove before/after calls in init
7dde95a 
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
Closed
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
@trevnorris @Fishrock123
async-wrap: don't call init callback unnecessarily
a2b6650 
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
@trevnorris @Fishrock123
async-wrap: pass PROVIDER as first arg to init
c8b05e4 
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
@trevnorris @Fishrock123
async-wrap: set flags using functions
a703e07 
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
@trevnorris @Fishrock123
async-wrap: remove before/after calls in init
54ae094 
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>
@Qard Qard mentioned this pull request Jun 25, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@trevnorris @bnoordhuis @brendanashworth