Skip to content

AsyncWrap minor fixes#1614

Closed
trevnorris wants to merge 4 commits intonodejs:masterfrom
trevnorris:async-wrap-minor-fixes
Closed

AsyncWrap minor fixes#1614
trevnorris wants to merge 4 commits intonodejs:masterfrom
trevnorris:async-wrap-minor-fixes

Conversation

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented May 4, 2015

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 trevnorris force-pushed the async-wrap-minor-fixes branch from 72d62fb to d7e0c53 Compare May 4, 2015 22:29
@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.
@trevnorris trevnorris force-pushed the async-wrap-minor-fixes branch from d7e0c53 to 13e52fe Compare May 4, 2015 22:44
@bnoordhuis
Copy link
Member

bnoordhuis commented May 4, 2015

LGTM with a suggestion.

trevnorris added 3 commits May 4, 2015 17:02
@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 trevnorris force-pushed the async-wrap-minor-fixes branch from 13e52fe to eed38b3 Compare May 4, 2015 23:04
@trevnorris
Copy link
Contributor Author

trevnorris commented May 4, 2015

@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
Copy link
Member

@bnoordhuis bnoordhuis May 4, 2015

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

@trevnorris trevnorris May 5, 2015

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

bnoordhuis commented May 4, 2015

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

trevnorris commented May 5, 2015

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.

3 participants

@trevnorris @bnoordhuis @brendanashworth