From 17aafd7b8843be0150c5b34ae5796252ba8bf48d Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 11:00:47 +0200 Subject: [PATCH 1/5] async_hooks: simplify fatalError Implement fatalError like ClearFatalExceptionHandlers. --- lib/async_hooks.js | 23 +++++++++------------- test/async-hooks/test-callback-error.js | 8 ++++---- test/async-hooks/test-emit-before-after.js | 4 ++-- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index e1029c97a57eec..8a2459c72f534a 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -60,19 +60,12 @@ async_wrap.setupHooks({ init, destroy: emitDestroyN }); // Used to fatally abort the process if a callback throws. +// This is JavaScript version of the ClearFatalExceptionHandlers function +// in C++. function fatalError(e) { - if (typeof e.stack === 'string') { - process._rawDebug(e.stack); - } else { - const o = { message: e }; - Error.captureStackTrace(o, fatalError); - process._rawDebug(o.stack); - } - if (process.execArgv.some( - (e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) { - process.abort(); - } - process.exit(1); + process.domain = undefined; + process._events.uncaughtException = undefined; + throw e; } @@ -371,8 +364,10 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { // Validate the ids. if (asyncId < 0 || triggerAsyncId < 0) { - fatalError('before(): asyncId or triggerAsyncId is less than zero ' + - `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})`); + fatalError(new Error( + 'before(): asyncId or triggerAsyncId is less than zero ' + + `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})` + )); } pushAsyncIds(asyncId, triggerAsyncId); diff --git a/test/async-hooks/test-callback-error.js b/test/async-hooks/test-callback-error.js index b110e700e61e4c..74e72cf507df58 100644 --- a/test/async-hooks/test-callback-error.js +++ b/test/async-hooks/test-callback-error.js @@ -50,7 +50,7 @@ assert.ok(!arg); console.time('end case 1'); const child = spawnSync(process.execPath, [__filename, 'test_init_callback']); assert.ifError(child.error); - const test_init_first_line = child.stderr.toString().split(/[\r\n]+/g)[0]; + const test_init_first_line = child.stderr.toString().split(/[\r\n]+/g)[3]; assert.strictEqual(test_init_first_line, 'Error: test_init_callback'); assert.strictEqual(child.status, 1); console.timeEnd('end case 1'); @@ -61,7 +61,7 @@ assert.ok(!arg); console.time('end case 2'); const child = spawnSync(process.execPath, [__filename, 'test_callback']); assert.ifError(child.error); - const test_callback_first_line = child.stderr.toString().split(/[\r\n]+/g)[0]; + const test_callback_first_line = child.stderr.toString().split(/[\r\n]+/g)[3]; assert.strictEqual(test_callback_first_line, 'Error: test_callback'); assert.strictEqual(child.status, 1); console.timeEnd('end case 2'); @@ -114,7 +114,7 @@ assert.ok(!arg); } assert.strictEqual(stdout, ''); const firstLineStderr = stderr.split(/[\r\n]+/g)[0].trim(); - assert.strictEqual(firstLineStderr, 'Error: test_callback_abort'); + assert.strictEqual(firstLineStderr, 'Uncaught Error: test_callback_abort'); + console.timeEnd('end case 3'); }); - console.timeEnd('end case 3'); } diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index f4757a28876acc..a0d0d4147706fe 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -16,13 +16,13 @@ switch (process.argv[2]) { } const c1 = spawnSync(process.execPath, [__filename, 'test_invalid_async_id']); -assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[0], +assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[3], 'Error: before(): asyncId or triggerAsyncId is less than ' + 'zero (asyncId: -1, triggerAsyncId: -1)'); assert.strictEqual(c1.status, 1); const c2 = spawnSync(process.execPath, [__filename, 'test_invalid_trigger_id']); -assert.strictEqual(c2.stderr.toString().split(/[\r\n]+/g)[0], +assert.strictEqual(c2.stderr.toString().split(/[\r\n]+/g)[3], 'Error: before(): asyncId or triggerAsyncId is less than ' + 'zero (asyncId: 1, triggerAsyncId: -1)'); assert.strictEqual(c2.status, 1); From 278b1636c91ef48424cad46fe4c43201c5f780cc Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 12:42:15 +0200 Subject: [PATCH 2/5] [squash] use try finally, for better stack traces --- lib/async_hooks.js | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 8a2459c72f534a..69199da51b88fe 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -62,10 +62,9 @@ async_wrap.setupHooks({ init, // Used to fatally abort the process if a callback throws. // This is JavaScript version of the ClearFatalExceptionHandlers function // in C++. -function fatalError(e) { +function clearFatalExceptionHandlers(e) { process.domain = undefined; process._events.uncaughtException = undefined; - throw e; } @@ -338,6 +337,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { function emitBeforeN(asyncId) { processing_hook = true; + var didThrow = true; // Use a single try/catch for all hook to avoid setting up one per iteration. try { for (var i = 0; i < active_hooks_array.length; i++) { @@ -345,8 +345,9 @@ function emitBeforeN(asyncId) { active_hooks_array[i][before_symbol](asyncId); } } - } catch (e) { - fatalError(e); + didThrow = false; + } finally { + if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -364,10 +365,11 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { // Validate the ids. if (asyncId < 0 || triggerAsyncId < 0) { - fatalError(new Error( + clearFatalExceptionHandlers(); + throw new Error( 'before(): asyncId or triggerAsyncId is less than zero ' + `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})` - )); + ); } pushAsyncIds(asyncId, triggerAsyncId); @@ -382,6 +384,7 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { // this is called. function emitAfterN(asyncId) { processing_hook = true; + var didThrow = true; // Use a single try/catch for all hook to avoid setting up one per iteration. try { for (var i = 0; i < active_hooks_array.length; i++) { @@ -389,8 +392,9 @@ function emitAfterN(asyncId) { active_hooks_array[i][after_symbol](asyncId); } } + didThrow = false; } catch (e) { - fatalError(e); + if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -422,6 +426,7 @@ function emitDestroyS(asyncId) { function emitDestroyN(asyncId) { processing_hook = true; + var didThrow = true; // Use a single try/catch for all hook to avoid setting up one per iteration. try { for (var i = 0; i < active_hooks_array.length; i++) { @@ -429,8 +434,9 @@ function emitDestroyN(asyncId) { active_hooks_array[i][destroy_symbol](asyncId); } } - } catch (e) { - fatalError(e); + didThrow = false; + } finally { + if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -455,6 +461,7 @@ function emitDestroyN(asyncId) { // exceptions. function init(asyncId, type, triggerAsyncId, resource) { processing_hook = true; + var didThrow = true; // Use a single try/catch for all hook to avoid setting up one per iteration. try { for (var i = 0; i < active_hooks_array.length; i++) { @@ -465,8 +472,8 @@ function init(asyncId, type, triggerAsyncId, resource) { ); } } - } catch (e) { - fatalError(e); + } finally { + if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; } From 610d3651a1236c763dee390c222640097661c859 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 13:15:50 +0200 Subject: [PATCH 3/5] [squash] ensure that uncaughtException listeners are removed --- test/async-hooks/test-callback-error.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/async-hooks/test-callback-error.js b/test/async-hooks/test-callback-error.js index 74e72cf507df58..00b91adfd4fadb 100644 --- a/test/async-hooks/test-callback-error.js +++ b/test/async-hooks/test-callback-error.js @@ -6,6 +6,10 @@ const async_hooks = require('async_hooks'); const initHooks = require('./init-hooks'); const arg = process.argv[2]; +if (arg) { + process.on('uncaughtException', common.mustNotCall()); +} + switch (arg) { case 'test_init_callback': initHooks({ @@ -114,7 +118,7 @@ assert.ok(!arg); } assert.strictEqual(stdout, ''); const firstLineStderr = stderr.split(/[\r\n]+/g)[0].trim(); - assert.strictEqual(firstLineStderr, 'Uncaught Error: test_callback_abort'); + assert.strictEqual(firstLineStderr, 'Error: test_callback_abort'); console.timeEnd('end case 3'); }); } From 1d3f69bd2fbc5e0190eac4d72255b5e401e6d57d Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 21:42:07 +0200 Subject: [PATCH 4/5] [squash] async_hooks c++ already have a try finally for each hook --- lib/async_hooks.js | 88 ++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 46 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 69199da51b88fe..5cdd805b49da6d 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -326,7 +326,14 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) throw new RangeError('triggerAsyncId must be an unsigned integer'); - init(asyncId, type, triggerAsyncId, resource); + // Remove prepear for a fatal exception in case an error occurred + var didThrow = true; + try { + init(asyncId, type, triggerAsyncId, resource); + didThrow = false; + } finally { + if (didThrow) clearFatalExceptionHandlers(); + } // Isn't null if hooks were added/removed while the hooks were running. if (tmp_active_hooks_array !== null) { @@ -337,17 +344,10 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { function emitBeforeN(asyncId) { processing_hook = true; - var didThrow = true; - // Use a single try/catch for all hook to avoid setting up one per iteration. - try { - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][before_symbol] === 'function') { - active_hooks_array[i][before_symbol](asyncId); - } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][before_symbol] === 'function') { + active_hooks_array[i][before_symbol](asyncId); } - didThrow = false; - } finally { - if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -376,7 +376,15 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { if (async_hook_fields[kBefore] === 0) return; - emitBeforeN(asyncId); + + // Remove prepear for a fatal exception in case an error occurred + var didThrow = true; + try { + emitBeforeN(asyncId); + didThrow = false; + } finally { + if (didThrow) clearFatalExceptionHandlers(); + } } @@ -384,17 +392,10 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) { // this is called. function emitAfterN(asyncId) { processing_hook = true; - var didThrow = true; - // Use a single try/catch for all hook to avoid setting up one per iteration. - try { - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][after_symbol] === 'function') { - active_hooks_array[i][after_symbol](asyncId); - } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][after_symbol] === 'function') { + active_hooks_array[i][after_symbol](asyncId); } - didThrow = false; - } catch (e) { - if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -408,8 +409,16 @@ function emitAfterN(asyncId) { // kIdStackIndex. But what happens if the user doesn't have both before and // after callbacks. function emitAfterS(asyncId) { - if (async_hook_fields[kAfter] > 0) - emitAfterN(asyncId); + if (async_hook_fields[kAfter] > 0) { + // Remove prepear for a fatal exception in case an error occurred + var didThrow = true; + try { + emitAfterN(asyncId); + didThrow = false; + } finally { + if (didThrow) clearFatalExceptionHandlers(); + } + } popAsyncIds(asyncId); } @@ -426,17 +435,10 @@ function emitDestroyS(asyncId) { function emitDestroyN(asyncId) { processing_hook = true; - var didThrow = true; - // Use a single try/catch for all hook to avoid setting up one per iteration. - try { - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][destroy_symbol] === 'function') { - active_hooks_array[i][destroy_symbol](asyncId); - } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][destroy_symbol] === 'function') { + active_hooks_array[i][destroy_symbol](asyncId); } - didThrow = false; - } finally { - if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; @@ -461,19 +463,13 @@ function emitDestroyN(asyncId) { // exceptions. function init(asyncId, type, triggerAsyncId, resource) { processing_hook = true; - var didThrow = true; - // Use a single try/catch for all hook to avoid setting up one per iteration. - try { - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][init_symbol] === 'function') { - active_hooks_array[i][init_symbol]( - asyncId, type, triggerAsyncId, - resource - ); - } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][init_symbol] === 'function') { + active_hooks_array[i][init_symbol]( + asyncId, type, triggerAsyncId, + resource + ); } - } finally { - if (didThrow) clearFatalExceptionHandlers(); } processing_hook = false; } From 527a4f687384d41b36d28189f61b67b4998f9582 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 21:43:55 +0200 Subject: [PATCH 5/5] [squash] add frindly comment explaining _events interaction --- lib/async_hooks.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 5cdd805b49da6d..f70a2e2080ac69 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -64,6 +64,8 @@ async_wrap.setupHooks({ init, // in C++. function clearFatalExceptionHandlers(e) { process.domain = undefined; + // Don't use removeAllListeners in case there is a removeListener listener + // that would revert it. process._events.uncaughtException = undefined; }