From 460cc6285a2144888e9433e3f5f7aa4f7a7eee6f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 2 Jun 2019 19:11:19 +0200 Subject: [PATCH] process: code cleanup for nextTick Fix a few edge cases and non-obvious issues with nextTick: 1. Emit destroy hook in a try-finally rather than triggering it before the callback runs. 2. Re-word comment for processPromiseRejections and make sure it returns true in the rejectionHandled case too. 3. Small readability improvements. PR-URL: https://github.com/nodejs/node/pull/28047 Reviewed-By: Ruben Bridgewater Reviewed-By: Joyee Cheung Reviewed-By: Jeremiah Senkpiel Reviewed-By: Rich Trott --- lib/internal/process/promises.js | 13 +++--- lib/internal/process/task_queues.js | 40 ++++++++---------- test/async-hooks/test-late-hook-enable.js | 49 +++++++++++++++++++++++ 3 files changed, 73 insertions(+), 29 deletions(-) create mode 100644 test/async-hooks/test-late-hook-enable.js diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 95161ac7a88ed8..324541551ef8b3 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -139,9 +139,12 @@ function emitDeprecationWarning() { } } -// If this method returns true, at least one more tick need to be -// scheduled to process any potential pending rejections +// If this method returns true, we've executed user code or triggered +// a warning to be emitted which requires the microtask and next tick +// queues to be drained again. function processPromiseRejections() { + let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0; + while (asyncHandledRejections.length > 0) { const { promise, warning } = asyncHandledRejections.shift(); if (!process.emit('rejectionHandled', promise)) { @@ -149,7 +152,6 @@ function processPromiseRejections() { } } - let maybeScheduledTicks = false; let len = pendingUnhandledRejections.length; while (len--) { const promise = pendingUnhandledRejections.shift(); @@ -167,9 +169,10 @@ function processPromiseRejections() { state === states.warn) { emitWarning(uid, reason); } - maybeScheduledTicks = true; + maybeScheduledTicksOrMicrotasks = true; } - return maybeScheduledTicks || pendingUnhandledRejections.length !== 0; + return maybeScheduledTicksOrMicrotasks || + pendingUnhandledRejections.length !== 0; } function getError(name, message) { diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js index 51486284578bce..1f203c86ed522f 100644 --- a/lib/internal/process/task_queues.js +++ b/lib/internal/process/task_queues.js @@ -65,46 +65,38 @@ function processTicksAndRejections() { while (tock = queue.shift()) { const asyncId = tock[async_id_symbol]; emitBefore(asyncId, tock[trigger_async_id_symbol]); - // emitDestroy() places the async_id_symbol into an asynchronous queue - // that calls the destroy callback in the future. It's called before - // calling tock.callback so destroy will be called even if the callback - // throws an exception that is handled by 'uncaughtException' or a - // domain. - // TODO(trevnorris): This is a bit of a hack. It relies on the fact - // that nextTick() doesn't allow the event loop to proceed, but if - // any async hooks are enabled during the callback's execution then - // this tock's after hook will be called, but not its destroy hook. - if (destroyHooksExist()) - emitDestroy(asyncId); - - const callback = tock.callback; - if (tock.args === undefined) - callback(); - else - callback(...tock.args); + + try { + const callback = tock.callback; + if (tock.args === undefined) + callback(); + else + callback(...tock.args); + } finally { + if (destroyHooksExist()) + emitDestroy(asyncId); + } emitAfter(asyncId); } - setHasTickScheduled(false); runMicrotasks(); } while (!queue.isEmpty() || processPromiseRejections()); + setHasTickScheduled(false); setHasRejectionToWarn(false); } class TickObject { - constructor(callback, args, triggerAsyncId) { + constructor(callback, args) { this.callback = callback; this.args = args; const asyncId = newAsyncId(); + const triggerAsyncId = getDefaultTriggerAsyncId(); this[async_id_symbol] = asyncId; this[trigger_async_id_symbol] = triggerAsyncId; if (initHooksExist()) { - emitInit(asyncId, - 'TickObject', - triggerAsyncId, - this); + emitInit(asyncId, 'TickObject', triggerAsyncId, this); } } } @@ -132,7 +124,7 @@ function nextTick(callback) { if (queue.isEmpty()) setHasTickScheduled(true); - queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId())); + queue.push(new TickObject(callback, args)); } let AsyncResource; diff --git a/test/async-hooks/test-late-hook-enable.js b/test/async-hooks/test-late-hook-enable.js new file mode 100644 index 00000000000000..0ffa6b24a3077d --- /dev/null +++ b/test/async-hooks/test-late-hook-enable.js @@ -0,0 +1,49 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +// Checks that enabling async hooks in a callback actually +// triggers after & destroy as expected. + +const fnsToTest = [setTimeout, (cb) => { + setImmediate(() => { + cb(); + + // We need to keep the event loop open for this to actually work + // since destroy hooks are triggered in unrefed Immediates + setImmediate(() => { + hook.disable(); + }); + }); +}, (cb) => { + process.nextTick(() => { + cb(); + + // We need to keep the event loop open for this to actually work + // since destroy hooks are triggered in unrefed Immediates + setImmediate(() => { + hook.disable(); + assert.strictEqual(fnsToTest.length, 0); + }); + }); +}]; + +const hook = async_hooks.createHook({ + before: common.mustNotCall(), + after: common.mustCall(() => {}, 3), + destroy: common.mustCall(() => { + hook.disable(); + nextTest(); + }, 3) +}); + +nextTest(); + +function nextTest() { + if (fnsToTest.length > 0) { + fnsToTest.shift()(common.mustCall(() => { + hook.enable(); + })); + } +}