Skip to content

Commit 44ea525

Browse files
trevnorrisMylesBorins
authored andcommitted
src: clear async id stack if bootstrap throws
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]>
1 parent 1bc0c1f commit 44ea525

File tree

2 files changed

+31
-1
lines changed

2 files changed

+31
-1
lines changed

src/node.cc

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3727,7 +3727,16 @@ void LoadEnvironment(Environment* env) {
37273727
// who do not like how bootstrap_node.js sets up the module system but do
37283728
// like Node's I/O bindings may want to replace 'f' with their own function.
37293729
Local<Value> arg = env->process_object();
3730-
f->Call(Null(env->isolate()), 1, &arg);
3730+
auto ret = f->Call(env->context(), Null(env->isolate()), 1, &arg);
3731+
// If there was an error during bootstrap then it was either handled by the
3732+
// FatalException handler or it's unrecoverable (e.g. max call stack
3733+
// exceeded). Either way, clear the stack so that the AsyncCallbackScope
3734+
// destructor doesn't fail on the id check.
3735+
// There are only two ways to have a stack size > 1: 1) the user manually
3736+
// called MakeCallback or 2) user awaited during bootstrap, which triggered
3737+
// _tickCallback().
3738+
if (ret.IsEmpty())
3739+
env->async_hooks()->clear_async_id_stack();
37313740
}
37323741

37333742
static void PrintHelp() {
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
'use strict';
2+
3+
require('../common');
4+
5+
if (process.argv[2] === 'async') {
6+
async function fn() {
7+
fn();
8+
throw new Error();
9+
}
10+
(async function() { await fn(); })();
11+
// While the above should error, just in case it dosn't the script shouldn't
12+
// fork itself indefinitely so return early.
13+
return;
14+
}
15+
16+
const assert = require('assert');
17+
const { spawnSync } = require('child_process');
18+
19+
const ret = spawnSync(process.execPath, [__filename, 'async']);
20+
assert.strictEqual(ret.status, 0);
21+
assert.ok(!/async.*hook/i.test(ret.stderr.toString('utf8', 0, 1024)));

0 commit comments

Comments
 (0)