Skip to content

Commit

Permalink
process: code cleanup for nextTick
Browse files Browse the repository at this point in the history
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: #28047
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
apapirovski authored and BridgeAR committed Jun 17, 2019
1 parent d4bb88e commit 460cc62
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 29 deletions.
13 changes: 8 additions & 5 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,19 @@ 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)) {
process.emitWarning(warning);
}
}

let maybeScheduledTicks = false;
let len = pendingUnhandledRejections.length;
while (len--) {
const promise = pendingUnhandledRejections.shift();
Expand All @@ -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) {
Expand Down
40 changes: 16 additions & 24 deletions lib/internal/process/task_queues.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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;
Expand Down
49 changes: 49 additions & 0 deletions test/async-hooks/test-late-hook-enable.js
Original file line number Diff line number Diff line change
@@ -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();
}));
}
}

0 comments on commit 460cc62

Please sign in to comment.