[async_wrap] backport of gc abort on destroy() fix on v6.x #10096
Conversation
The constructor and destructor shouldn't have been placed in the -inl.h file from the beginning. Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This is how it's done everywhere else in core. Make it follow suit. Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Calling JS during GC is a no-no. So intead create a queue of all ids that need to have their destroy() callback called and call them later. Removed checking destroy() in test-async-wrap-uid because destroy() can be called after the 'exit' callback. Missing a reliable test to reproduce the issue that caused the FATAL_ERROR. Fixes: nodejs#8216 Fixes: nodejs#9465 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-github-bot
added
c++
|
@nodejs/ctc @nodejs/lts are people opposed to this landing onto v6.x-staging and included in the release on Tuesday? |
|
Landing this on v6.x-staging and including it in the next release sounds good to me. I would guess this should also even happen for v4? |
|
To clarify... if we land this in v6.x on Tuesday it will have not been out in a release before landing in LTS. @trevnorris had suggested this would be reasonable as this change only affects users of an undocumented api |
mscdex
changed the title
|
There were 4 failures on smart os https://ci.nodejs.org/job/node-test-commit-smartos/5478/nodes=smartos15-64/tapTestReport/ @trevnorris are these related? /cc @nodejs/platform-smartos |
|
These don’t look related to me. Here’s another CI run: https://ci.nodejs.org/job/node-test-commit/6438/ |
|
Seems like a good idea to get this landed. LGTM |
|
landed in 59d8255...72595f7 |
The constructor and destructor shouldn't have been placed in the -inl.h file from the beginning. PR-URL: #10096 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This is how it's done everywhere else in core. Make it follow suit. PR-URL: #10096 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Calling JS during GC is a no-no. So intead create a queue of all ids that need to have their destroy() callback called and call them later. Removed checking destroy() in test-async-wrap-uid because destroy() can be called after the 'exit' callback. Missing a reliable test to reproduce the issue that caused the FATAL_ERROR. PR-URL: #10096 Fixes: #8216 Fixes: #9465 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
@thealphanerd This has a bug that's fixed by #10400. Leave it to me to make an seemingly innocuous change that totally fails. |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
async_wrap
Description of change
This is a backport of 517e3a6, cf5f4b8 and b49b496 to v6.x
This is semi-urgent. In the issues listed as
Fixedin the commit shows the process is aborting in v6. So we can weight the urgency of landing this vs the possible regression it may introduce. Note that there could only be a regression if the user is usingprocess.binding('async_wrap'). Otherwise all the changed logic is skipped.