Begin AsyncWrap maintenance#3139
Uh oh!
There was an error while loading. Please reload this page.
Conversation
trevnorris added the c++ Several provider ids have been removed that are no longer in use. Others have been updated to match their class constructors. Add test to ensure all internally listed providers are used.
trevnorrisforce-pushed the async-awesome branch from 31a59b8 to d415be9CompareIf the constructor can't assign a class id then the heap snapshot will not be able to report the object. So ensure that all AsyncWrap instances use a FunctionTemplate instance with an internal field count >= 1.
trevnorrisforce-pushed the async-awesome branch from d415be9 to 66b2890Compare
Fishrock123 commented Sep 30, 2015
To be clear, async wrap will still work during this time, correct? Otherwise one PR may be more appropriate? |
trevnorris commented Oct 1, 2015
@Fishrock123 Each PR will have a small set of fixes. No PR will prevent AsyncWrap from working at least as well as it does today. This PR fixed outdated providers list and not all instances being reported in the heap snapshot. |
Fishrock123 commented Oct 1, 2015
Sounds good to me then. I definitely can't actually review this haha. |
There was a problem hiding this comment.
These two will throw errors. Doesn't it make it major change? If this is an internal function, do we really need this?
There was a problem hiding this comment.
This doesn't throw an error. It aborts. The first couldn't have happened anyway because of how the function signature works. It's there as a sanity check for future development. The latter should never have been happening as it would have caused issues when iterating the heap.
thefourtheye commented Oct 1, 2015
Can you please summarize what this PR does and why it is necessary? Edit:
Ah I see. I'll try to find what providers are. |
There was a problem hiding this comment.
Mild cognitive dissonance here, the _t suffix makes it look like the variables are typedefs at a quick glance.
There was a problem hiding this comment.
Forgot that convention. Was using it as shorthand for template. Will change.
bnoordhuis commented Oct 1, 2015
LGTM with some suggestions. |
Qard commented Oct 1, 2015
LGTM, other than the comments from Ben. |
Several provider ids have been removed that are no longer in use. Others have been updated to match their class constructors. Add test to ensure all internally listed providers are used. PR-URL: #3139 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Stephen Belanger <[email protected]>
If the constructor can't assign a class id then the heap snapshot will not be able to report the object. So ensure that all AsyncWrap instances use a FunctionTemplate instance with an internal field count >= 1. PR-URL: #3139 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Stephen Belanger <[email protected]>
trevnorris commented Oct 1, 2015
Landed with suggested changes in e52864b and 3f476ad. Should land on current stable before going LTS. |
Several provider ids have been removed that are no longer in use. Others have been updated to match their class constructors. Add test to ensure all internally listed providers are used. PR-URL: #3139 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Stephen Belanger <[email protected]>
If the constructor can't assign a class id then the heap snapshot will not be able to report the object. So ensure that all AsyncWrap instances use a FunctionTemplate instance with an internal field count >= 1. PR-URL: #3139 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Stephen Belanger <[email protected]>
MylesBorins commented Nov 17, 2015
landed in v4.x-staging in e561585...39b8730 |
8 participants
This is long overdue maintenance on
AsyncWrap. More PRs will follow, but segmenting to keep reviews simple.R=@bnoordhuis
R=@indutny
CI: https://ci.nodejs.org/job/node-test-pull-request/405/CI: https://ci.nodejs.org/job/node-test-pull-request/406/