Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

async_hooks: cleanup emitBefore and emitAfter in timers and nextTick #14050

Closed
wants to merge 2 commits into from
Closed

async_hooks: cleanup emitBefore and emitAfter in timers and nextTick #14050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9b2206
async_hooks: use common emitBefore and emitAfter
AndreasMadsen Jul 2, 2017
ae5355e
async_hooks: require parameter in emitBefore
AndreasMadsen Jul 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ function emitHookFactory(symbol, name) {

// Usage: emitBeforeS(asyncId[, triggerAsyncId]). If triggerAsyncId is omitted
// then asyncId will be used instead.
function emitBeforeS(asyncId, triggerAsyncId = asyncId) {
function emitBeforeS(asyncId, triggerAsyncId) {
// CHECK(Number.isSafeInteger(asyncId) && asyncId > 0)
// CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0)

Expand All @@ -389,9 +389,8 @@ function emitBeforeS(asyncId, triggerAsyncId = asyncId) {

pushAsyncIds(asyncId, triggerAsyncId);

if (async_hook_fields[kBefore] === 0)
return;
emitBeforeNative(asyncId);
if (async_hook_fields[kBefore] > 0)
emitBeforeNative(asyncId);
}


Expand Down
31 changes: 5 additions & 26 deletions lib/internal/process/next_tick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,9 @@ function setupNextTick() {
// Two arrays that share state between C++ and JS.
const { async_hook_fields, async_uid_fields } = async_wrap;
// Used to change the state of the async id stack.
const { pushAsyncIds, popAsyncIds } = async_wrap;
// The needed emit*() functions.
const { emitInit, emitBefore, emitAfter, emitDestroy } = async_hooks;
// Grab the constants necessary for working with internal arrays.
const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr } =
async_wrap.constants;
const { kInit, kDestroy, kAsyncUidCntr } = async_wrap.constants;
const { async_id_symbol, trigger_id_symbol } = async_wrap;
var nextTickQueue = new NextTickQueue();
var microtasksScheduled = false;
Expand Down Expand Up @@ -149,24 +146,6 @@ function setupNextTick() {
}
}

// TODO(trevnorris): Using std::stack of Environment::AsyncHooks::ids_stack_
// is much slower here than was the Float64Array stack used in a previous
// implementation. Problem is the Float64Array stack was a bit brittle.
// Investigate how to harden that implementation and possibly reintroduce it.
function nextTickEmitBefore(asyncId, triggerAsyncId) {
if (async_hook_fields[kBefore] > 0)
emitBefore(asyncId, triggerAsyncId);
else
pushAsyncIds(asyncId, triggerAsyncId);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

History lesson: This was introduced originally because of a crazy method to keep track of the id stack. Which was a lot faster than it is currently, but added an obscene amount of complexity. I hope to simplify that approach in the future and reintroduce it, but seems I overlooked these calls can be removed for the time being.


function nextTickEmitAfter(asyncId) {
if (async_hook_fields[kAfter] > 0)
emitAfter(asyncId);
else
popAsyncIds(asyncId);
}

// Run callbacks that have no domain.
// Using domains will cause this to be overridden.
function _tickCallback() {
Expand All @@ -182,7 +161,7 @@ function setupNextTick() {
// CHECK(Number.isSafeInteger(tock[trigger_id_symbol]))
// CHECK(tock[trigger_id_symbol] > 0)

nextTickEmitBefore(tock[async_id_symbol], tock[trigger_id_symbol]);
emitBefore(tock[async_id_symbol], tock[trigger_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
Expand All @@ -200,7 +179,7 @@ function setupNextTick() {
// performance hit associated with using `fn.apply()`
_combinedTickCallback(args, callback);

nextTickEmitAfter(tock[async_id_symbol]);
emitAfter(tock[async_id_symbol]);

if (kMaxCallbacksPerLoop < tickInfo[kIndex])
tickDone();
Expand All @@ -227,7 +206,7 @@ function setupNextTick() {
// CHECK(Number.isSafeInteger(tock[trigger_id_symbol]))
// CHECK(tock[trigger_id_symbol] > 0)

nextTickEmitBefore(tock[async_id_symbol], tock[trigger_id_symbol]);
emitBefore(tock[async_id_symbol], tock[trigger_id_symbol]);
// TODO(trevnorris): See comment in _tickCallback() as to why this
// isn't a good solution.
if (async_hook_fields[kDestroy] > 0)
Expand All @@ -238,7 +217,7 @@ function setupNextTick() {
// performance hit associated with using `fn.apply()`
_combinedTickCallback(args, callback);

nextTickEmitAfter(tock[async_id_symbol]);
emitAfter(tock[async_id_symbol]);

if (kMaxCallbacksPerLoop < tickInfo[kIndex])
tickDone();
Expand Down
29 changes: 5 additions & 24 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,10 @@ const kOnTimeout = TimerWrap.kOnTimeout | 0;
const initTriggerId = async_hooks.initTriggerId;
// Two arrays that share state between C++ and JS.
const { async_hook_fields, async_uid_fields } = async_wrap;
// Used to change the state of the async id stack.
const { pushAsyncIds, popAsyncIds } = async_wrap;
// The needed emit*() functions.
const { emitInit, emitBefore, emitAfter, emitDestroy } = async_hooks;
// Grab the constants necessary for working with internal arrays.
const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr } =
async_wrap.constants;
const { kInit, kDestroy, kAsyncUidCntr } = async_wrap.constants;
// Symbols for storing async id state.
const async_id_symbol = Symbol('asyncId');
const trigger_id_symbol = Symbol('triggerAsyncId');
Expand Down Expand Up @@ -149,22 +146,6 @@ exports._unrefActive = function(item) {
};


function timerEmitBefore(asyncId, triggerAsyncId) {
if (async_hook_fields[kBefore] > 0)
emitBefore(asyncId, triggerAsyncId);
else
pushAsyncIds(asyncId, triggerAsyncId);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason these were here is for the same reason explained in nextTick.



function timerEmitAfter(asyncId) {
if (async_hook_fields[kAfter] > 0)
emitAfter(asyncId);
else
popAsyncIds(asyncId);
}


// The underlying logic for scheduling or re-scheduling a timer.
//
// Appends a timer onto the end of an existing timers list, or creates a new
Expand Down Expand Up @@ -318,14 +299,14 @@ function tryOnTimeout(timer, list) {
timer[async_id_symbol] : null;
var threw = true;
if (timerAsyncId !== null)
timerEmitBefore(timerAsyncId, timer[trigger_id_symbol]);
emitBefore(timerAsyncId, timer[trigger_id_symbol]);
try {
ontimeout(timer);
threw = false;
} finally {
if (timerAsyncId !== null) {
if (!threw)
timerEmitAfter(timerAsyncId);
emitAfter(timerAsyncId);
if (!timer._repeat && async_hook_fields[kDestroy] > 0 &&
!timer._destroyed) {
emitDestroy(timerAsyncId);
Expand Down Expand Up @@ -756,7 +737,7 @@ function processImmediate() {
// 4.7) what is in this smaller function.
function tryOnImmediate(immediate, oldTail) {
var threw = true;
timerEmitBefore(immediate[async_id_symbol], immediate[trigger_id_symbol]);
emitBefore(immediate[async_id_symbol], immediate[trigger_id_symbol]);
try {
// make the actual call outside the try/catch to allow it to be optimized
runCallback(immediate);
Expand All @@ -765,7 +746,7 @@ function tryOnImmediate(immediate, oldTail) {
// clearImmediate checks _callback === null for kDestroy hooks.
immediate._callback = null;
if (!threw)
timerEmitAfter(immediate[async_id_symbol]);
emitAfter(immediate[async_id_symbol]);
if (async_hook_fields[kDestroy] > 0 && !immediate._destroyed) {
emitDestroy(immediate[async_id_symbol]);
immediate._destroyed = true;
Expand Down
15 changes: 8 additions & 7 deletions test/async-hooks/test-callback-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,33 @@ switch (arg) {
oninit: common.mustCall(() => { throw new Error(arg); })
}).enable();
async_hooks.emitInit(
async_hooks.executionAsyncId(),
async_hooks.newUid(),
`${arg}_type`,
async_hooks.triggerAsyncId()
async_hooks.executionAsyncId()
);
return;

case 'test_callback':
initHooks({
onbefore: common.mustCall(() => { throw new Error(arg); })
}).enable();
const newAsyncId = async_hooks.newUid();
async_hooks.emitInit(
async_hooks.executionAsyncId(),
newAsyncId,
`${arg}_type`,
async_hooks.triggerAsyncId()
async_hooks.executionAsyncId()
);
async_hooks.emitBefore(async_hooks.executionAsyncId());
async_hooks.emitBefore(newAsyncId, async_hooks.executionAsyncId());
return;

case 'test_callback_abort':
initHooks({
oninit: common.mustCall(() => { throw new Error(arg); })
}).enable();
async_hooks.emitInit(
async_hooks.executionAsyncId(),
async_hooks.newUid(),
`${arg}_type`,
async_hooks.triggerAsyncId()
async_hooks.executionAsyncId()
);
return;
}
Expand Down
14 changes: 8 additions & 6 deletions test/async-hooks/test-emit-before-after.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,19 @@ const initHooks = require('./init-hooks');

switch (process.argv[2]) {
case 'test_invalid_async_id':
async_hooks.emitBefore(-1);
break;
async_hooks.emitBefore(-1, 1);
return;
case 'test_invalid_trigger_id':
async_hooks.emitBefore(1, -1);
break;
return;
}
assert.ok(!process.argv[2]);


const c1 = spawnSync(process.execPath, [__filename, 'test_invalid_async_id']);
assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[0],
'Error: before(): asyncId or triggerAsyncId is less than ' +
'zero (asyncId: -1, triggerAsyncId: -1)');
'zero (asyncId: -1, triggerAsyncId: 1)');
assert.strictEqual(c1.status, 1);

const c2 = spawnSync(process.execPath, [__filename, 'test_invalid_trigger_id']);
Expand All @@ -32,7 +34,7 @@ const expectedTriggerId = async_hooks.newUid();
const expectedType = 'test_emit_before_after_type';

// Verify that if there is no registered hook, then nothing will happen.
async_hooks.emitBefore(expectedId);
async_hooks.emitBefore(expectedId, expectedTriggerId);
async_hooks.emitAfter(expectedId);

initHooks({
Expand All @@ -42,5 +44,5 @@ initHooks({
}).enable();

async_hooks.emitInit(expectedId, expectedType, expectedTriggerId);
async_hooks.emitBefore(expectedId);
async_hooks.emitBefore(expectedId, expectedTriggerId);
async_hooks.emitAfter(expectedId);