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 <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This commit is contained in:
Anatoli Papirovski 2019-06-02 19:11:19 +02:00
parent fefc275dcb
commit abf765e33c
3 changed files with 72 additions and 28 deletions

View File

@ -139,9 +139,12 @@ function emitDeprecationWarning() {
} }
} }
// If this method returns true, at least one more tick need to be // If this method returns true, we've executed user code or triggered
// scheduled to process any potential pending rejections // a warning to be emitted which requires the microtask and next tick
// queues to be drained again.
function processPromiseRejections() { function processPromiseRejections() {
let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0;
while (asyncHandledRejections.length > 0) { while (asyncHandledRejections.length > 0) {
const { promise, warning } = asyncHandledRejections.shift(); const { promise, warning } = asyncHandledRejections.shift();
if (!process.emit('rejectionHandled', promise)) { if (!process.emit('rejectionHandled', promise)) {
@ -149,7 +152,6 @@ function processPromiseRejections() {
} }
} }
let maybeScheduledTicks = false;
let len = pendingUnhandledRejections.length; let len = pendingUnhandledRejections.length;
while (len--) { while (len--) {
const promise = pendingUnhandledRejections.shift(); const promise = pendingUnhandledRejections.shift();
@ -167,9 +169,10 @@ function processPromiseRejections() {
state === states.warn) { state === states.warn) {
emitWarning(uid, reason); emitWarning(uid, reason);
} }
maybeScheduledTicks = true; maybeScheduledTicksOrMicrotasks = true;
} }
return maybeScheduledTicks || pendingUnhandledRejections.length !== 0; return maybeScheduledTicksOrMicrotasks ||
pendingUnhandledRejections.length !== 0;
} }
function getError(name, message) { function getError(name, message) {

View File

@ -65,46 +65,38 @@ function processTicksAndRejections() {
while (tock = queue.shift()) { while (tock = queue.shift()) {
const asyncId = tock[async_id_symbol]; const asyncId = tock[async_id_symbol];
emitBefore(asyncId, tock[trigger_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; try {
if (tock.args === undefined) const callback = tock.callback;
callback(); if (tock.args === undefined)
else callback();
callback(...tock.args); else
callback(...tock.args);
} finally {
if (destroyHooksExist())
emitDestroy(asyncId);
}
emitAfter(asyncId); emitAfter(asyncId);
} }
setHasTickScheduled(false);
runMicrotasks(); runMicrotasks();
} while (!queue.isEmpty() || processPromiseRejections()); } while (!queue.isEmpty() || processPromiseRejections());
setHasTickScheduled(false);
setHasRejectionToWarn(false); setHasRejectionToWarn(false);
} }
class TickObject { class TickObject {
constructor(callback, args, triggerAsyncId) { constructor(callback, args) {
this.callback = callback; this.callback = callback;
this.args = args; this.args = args;
const asyncId = newAsyncId(); const asyncId = newAsyncId();
const triggerAsyncId = getDefaultTriggerAsyncId();
this[async_id_symbol] = asyncId; this[async_id_symbol] = asyncId;
this[trigger_async_id_symbol] = triggerAsyncId; this[trigger_async_id_symbol] = triggerAsyncId;
if (initHooksExist()) { if (initHooksExist()) {
emitInit(asyncId, emitInit(asyncId, 'TickObject', triggerAsyncId, this);
'TickObject',
triggerAsyncId,
this);
} }
} }
} }
@ -132,7 +124,7 @@ function nextTick(callback) {
if (queue.isEmpty()) if (queue.isEmpty())
setHasTickScheduled(true); setHasTickScheduled(true);
queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId())); queue.push(new TickObject(callback, args));
} }
let AsyncResource; let AsyncResource;

View File

@ -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();
}));
}
}