From 1a66c0dd41cbf8fb9b05d47a54d7fa73b4103b53 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 2 Jun 2019 19:11:19 +0200 Subject: [PATCH 1/5] 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. --- lib/internal/process/promises.js | 13 ++++++---- lib/internal/process/task_queues.js | 40 ++++++++++++----------------- 2 files changed, 24 insertions(+), 29 deletions(-) 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; From 8c8c7309ca61a06c3b259b80c4ff8f30e024ca9c Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 5 Jun 2019 10:44:37 +0200 Subject: [PATCH 2/5] fixup: add destroy hook test --- test/async-hooks/test-late-hook-enable.js | 36 +++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 test/async-hooks/test-late-hook-enable.js 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..cab221ae2e3985 --- /dev/null +++ b/test/async-hooks/test-late-hook-enable.js @@ -0,0 +1,36 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +const fnsToTest = [setTimeout, setImmediate, (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(); + })); + } +} From c5c3c6fd9c62e9bedea9d4507aaf1158a20bee3f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 5 Jun 2019 12:34:56 +0200 Subject: [PATCH 3/5] fixup: add comment explaining the test --- test/async-hooks/test-late-hook-enable.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/async-hooks/test-late-hook-enable.js b/test/async-hooks/test-late-hook-enable.js index cab221ae2e3985..b72e5ef17147a7 100644 --- a/test/async-hooks/test-late-hook-enable.js +++ b/test/async-hooks/test-late-hook-enable.js @@ -3,6 +3,9 @@ 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, setImmediate, (cb) => { process.nextTick(() => { cb(); From 20f2a93b55a83c8c5faf678732f395f060df2783 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 7 Jun 2019 09:38:31 +0200 Subject: [PATCH 4/5] fixup: make test work in workers --- test/async-hooks/test-late-hook-enable.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/async-hooks/test-late-hook-enable.js b/test/async-hooks/test-late-hook-enable.js index b72e5ef17147a7..13a74aa00492fe 100644 --- a/test/async-hooks/test-late-hook-enable.js +++ b/test/async-hooks/test-late-hook-enable.js @@ -6,7 +6,17 @@ const async_hooks = require('async_hooks'); // Checks that enabling async hooks in a callback actually // triggers after & destroy as expected. -const fnsToTest = [setTimeout, setImmediate, (cb) => { +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(); From 83658f79569e7acb305e6f36f751c23a632baf5c Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 7 Jun 2019 09:51:30 +0200 Subject: [PATCH 5/5] fixup: typo --- test/async-hooks/test-late-hook-enable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/async-hooks/test-late-hook-enable.js b/test/async-hooks/test-late-hook-enable.js index 13a74aa00492fe..0ffa6b24a3077d 100644 --- a/test/async-hooks/test-late-hook-enable.js +++ b/test/async-hooks/test-late-hook-enable.js @@ -15,7 +15,7 @@ const fnsToTest = [setTimeout, (cb) => { setImmediate(() => { hook.disable(); }); - }) + }); }, (cb) => { process.nextTick(() => { cb();