src: clear async id stack if bootstrap throws #15553

trevnorris · 2017-09-22T08:17:19Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

Notes

If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

async function fn() {
  fn();
  throw new Error();
}
(async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

(node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

script.js:25
async function fn() {
                 ^
RangeError: Maximum call stack size exceeded
    at <anonymous>
    at fn (script.js:25:18)
    at fn (script.js:26:3)
    ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: #15448

Note: Still need to add my own test

addaleax

It would be nice if you could add the crashing script as a test case here

trevnorris · 2017-09-26T21:01:27Z

@addaleax I've added the test. running CI once more

CI: https://ci.nodejs.org/job/node-test-pull-request/10281/

If bootstrap throws and if ids are added to the async id stack and if the exception wasn't handled by the fatal exception handler then the AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack check to fail. Causing the application to crash. So clear the async id stack manually. This is only possible if the user: 1) manually calls MakeCallback() or 2) uses async await in the top level. Which will cause _tickCallback() to fire before bootstrap finishes executing. The following example shows how the application can fail due to exceeding the maximum call stack while using async await: async function fn() { fn(); throw new Error(); } (async function() { await fn(); })(); If this occurs during bootstrap then the application will pring the following warning a number of times then exit with a status of 0: (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error Here's the same recursive call done after enabling a new AsyncHook() the following will print instead of the above warning and exit with a non-zero code (currently it's 7 because of how node::FatalException assigns error codes based on where the failure happened): script.js:25 async function fn() { ^ RangeError: Maximum call stack size exceeded at <anonymous> at fn (script.js:25:18) at fn (script.js:26:3) .... This has to do with how Promises lazily enable PromiseHook if an AsyncHook() is enabled. Whether these need to be made uniform is outside the scope of this commit Fixes: nodejs#15448 PR-URL: nodejs#15553 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

trevnorris · 2017-09-27T08:11:23Z

Forgot to change the API after rebase.

CI: https://ci.nodejs.org/job/node-test-commit/12630/

If bootstrap throws and if ids are added to the async id stack and if the exception wasn't handled by the fatal exception handler then the AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack check to fail. Causing the application to crash. So clear the async id stack manually. This is only possible if the user: 1) manually calls MakeCallback() or 2) uses async await in the top level. Which will cause _tickCallback() to fire before bootstrap finishes executing. The following example shows how the application can fail due to exceeding the maximum call stack while using async await: async function fn() { fn(); throw new Error(); } (async function() { await fn(); })(); If this occurs during bootstrap then the application will pring the following warning a number of times then exit with a status of 0: (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error Here's the same recursive call done after enabling a new AsyncHook() the following will print instead of the above warning and exit with a non-zero code (currently it's 7 because of how node::FatalException assigns error codes based on where the failure happened): script.js:25 async function fn() { ^ RangeError: Maximum call stack size exceeded at <anonymous> at fn (script.js:25:18) at fn (script.js:26:3) .... This has to do with how Promises lazily enable PromiseHook if an AsyncHook() is enabled. Whether these need to be made uniform is outside the scope of this commit Fixes: #15448 PR-URL: #15553 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

If bootstrap throws and if ids are added to the async id stack and if the exception wasn't handled by the fatal exception handler then the AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack check to fail. Causing the application to crash. So clear the async id stack manually. This is only possible if the user: 1) manually calls MakeCallback() or 2) uses async await in the top level. Which will cause _tickCallback() to fire before bootstrap finishes executing. The following example shows how the application can fail due to exceeding the maximum call stack while using async await: async function fn() { fn(); throw new Error(); } (async function() { await fn(); })(); If this occurs during bootstrap then the application will pring the following warning a number of times then exit with a status of 0: (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error Here's the same recursive call done after enabling a new AsyncHook() the following will print instead of the above warning and exit with a non-zero code (currently it's 7 because of how node::FatalException assigns error codes based on where the failure happened): script.js:25 async function fn() { ^ RangeError: Maximum call stack size exceeded at <anonymous> at fn (script.js:25:18) at fn (script.js:26:3) .... This has to do with how Promises lazily enable PromiseHook if an AsyncHook() is enabled. Whether these need to be made uniform is outside the scope of this commit Fixes: nodejs/node#15448 PR-URL: nodejs/node#15553 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

If bootstrap throws and if ids are added to the async id stack and if the exception wasn't handled by the fatal exception handler then the AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack check to fail. Causing the application to crash. So clear the async id stack manually. This is only possible if the user: 1) manually calls MakeCallback() or 2) uses async await in the top level. Which will cause _tickCallback() to fire before bootstrap finishes executing. The following example shows how the application can fail due to exceeding the maximum call stack while using async await: async function fn() { fn(); throw new Error(); } (async function() { await fn(); })(); If this occurs during bootstrap then the application will pring the following warning a number of times then exit with a status of 0: (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error Here's the same recursive call done after enabling a new AsyncHook() the following will print instead of the above warning and exit with a non-zero code (currently it's 7 because of how node::FatalException assigns error codes based on where the failure happened): script.js:25 async function fn() { ^ RangeError: Maximum call stack size exceeded at <anonymous> at fn (script.js:25:18) at fn (script.js:26:3) .... This has to do with how Promises lazily enable PromiseHook if an AsyncHook() is enabled. Whether these need to be made uniform is outside the scope of this commit Fixes: #15448 PR-URL: #15553 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-10-17T18:59:03Z

this lands cleanly on v6.x, should it be backported?

MylesBorins · 2017-11-14T20:29:24Z

ping @trevnorris

MylesBorins · 2017-12-19T17:31:06Z

ping @trevnorris

MylesBorins · 2018-01-11T18:23:20Z

ping @trevnorris

trevnorris requested review from bnoordhuis and addaleax September 22, 2017 08:17

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 22, 2017

trevnorris mentioned this pull request Sep 22, 2017

async_hooks: async hook stack assertion shadows "Maximum call stack size exceeded" #15448

Closed

addaleax approved these changes Sep 22, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

trevnorris force-pushed the bad-stack-bootstrap branch from f1a97ea to 91faee2 Compare September 26, 2017 21:00

refack approved these changes Sep 27, 2017

View reviewed changes

trevnorris force-pushed the bad-stack-bootstrap branch from 91faee2 to 92d7e81 Compare September 27, 2017 08:10

trevnorris merged commit 92d7e81 into nodejs:master Sep 27, 2017

trevnorris deleted the bad-stack-bootstrap branch September 27, 2017 10:42

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the lts-watch-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: clear async id stack if bootstrap throws #15553

src: clear async id stack if bootstrap throws #15553

trevnorris commented Sep 22, 2017 •

edited

Loading

addaleax left a comment

trevnorris commented Sep 26, 2017

trevnorris commented Sep 27, 2017 •

edited

Loading

MylesBorins commented Oct 17, 2017

MylesBorins commented Nov 14, 2017

MylesBorins commented Dec 19, 2017

MylesBorins commented Jan 11, 2018

src: clear async id stack if bootstrap throws #15553

src: clear async id stack if bootstrap throws #15553

Conversation

trevnorris commented Sep 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Notes

addaleax left a comment

Choose a reason for hiding this comment

trevnorris commented Sep 26, 2017

trevnorris commented Sep 27, 2017 • edited Loading

MylesBorins commented Oct 17, 2017

MylesBorins commented Nov 14, 2017

MylesBorins commented Dec 19, 2017

MylesBorins commented Jan 11, 2018

trevnorris commented Sep 22, 2017 •

edited

Loading

trevnorris commented Sep 27, 2017 •

edited

Loading