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: fix nested hooks mutation #14143

Closed
wants to merge 7 commits into from
Closed

async_hooks: fix nested hooks mutation #14143

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a4cca58
async_hooks: move restoreTmpHooks call to init
BridgeAR Jul 3, 2017
19890ce
fixup: use fs instead of crypto
BridgeAR Jul 11, 2017
3e5d7ba
async_hooks: fix nested hooks mutation
AndreasMadsen Jul 9, 2017
65d4760
[squash] lint
AndreasMadsen Jul 10, 2017
d04802b
[squash] don't use crypto
AndreasMadsen Jul 10, 2017
0b80a31
[squash] use finally block
AndreasMadsen Jul 10, 2017
cbd1f78
[squash] update comment
AndreasMadsen Jul 13, 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
32 changes: 20 additions & 12 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ var active_hooks_array = [];
// Track whether a hook callback is currently being processed. Used to make
// sure active_hooks_array isn't altered in mid execution if another hook is
// added or removed.
var processing_hook = false;
var processing_hook = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly change comment above from simply

Track whether a hook [...]

to something like

Use a counter to track [...] and prevent nested calls [...]

// Use to temporarily store and updated active_hooks_array if the user enables
// or disables a hook while hooks are being processed.
var tmp_active_hooks_array = null;
Expand Down Expand Up @@ -151,7 +151,7 @@ class AsyncHook {


function getHookArrays() {
if (!processing_hook)
if (processing_hook === 0)
return [active_hooks_array, async_hook_fields];
// If this hook is being enabled while in the middle of processing the array
// of currently active hooks then duplicate the current set of active hooks
Expand Down Expand Up @@ -335,19 +335,14 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
throw new RangeError('triggerAsyncId must be an unsigned integer');

init(asyncId, type, triggerAsyncId, resource);

// Isn't null if hooks were added/removed while the hooks were running.
if (tmp_active_hooks_array !== null) {
restoreTmpHooks();
}
}

function emitHookFactory(symbol, name) {
// Called from native. The asyncId stack handling is taken care of there
// before this is called.
// eslint-disable-next-line func-style
const fn = function(asyncId) {
processing_hook = true;
processing_hook += 1;
// Use a single try/catch for all hook to avoid setting up one per
// iteration.
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that doing reference counting is too fragile.
Why not replace the for either with a .forEach for with a for (const hook of Array.from(active_hooks_array))

Copy link
Member Author

@AndreasMadsen AndreasMadsen Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm worried that doing reference counting is too fragile.

Could you be more explicit?

edit: Just to avoid confusion, this is not a reference counting it is a depth counting.

Why not replace the for either with a .forEach

.forEach is not resistant to mutation.

const a = [1,2,3];
a.forEach(function (value) {
  process._rawDebug(value);
  a.pop();
});

prints 1 2.

with a for (const hook of Array.from(active_hooks_array))

Array copy is too slow. When async_hooks is enabled this is the hottest code path we have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack. I was sure .forEach iterated over a fixed array (it just ignores newly added elements)

Expand All @@ -358,10 +353,11 @@ function emitHookFactory(symbol, name) {
}
} catch (e) {
fatalError(e);
} finally {
processing_hook -= 1;
}
processing_hook = false;

if (tmp_active_hooks_array !== null) {
if (processing_hook === 0 && tmp_active_hooks_array !== null) {
restoreTmpHooks();
}
};
Expand Down Expand Up @@ -427,7 +423,7 @@ function emitDestroyS(asyncId) {
// slim chance of the application remaining stable after handling one of these
// exceptions.
function init(asyncId, type, triggerAsyncId, resource) {
processing_hook = true;
processing_hook += 1;
// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
for (var i = 0; i < active_hooks_array.length; i++) {
Expand All @@ -440,8 +436,20 @@ function init(asyncId, type, triggerAsyncId, resource) {
}
} catch (e) {
fatalError(e);
} finally {
processing_hook -= 1;
}

// * `tmp_active_hooks_array` is null if no hooks were added/removed while
// the hooks were running. In that case no restoration is needed.
// * In the case where another hook was added/removed while the hooks were
// running and a handle was created causing the `init` hooks to fire again,
// then `restoreTmpHooks` should not be called for the nested `hooks`.
// Otherwise `active_hooks_array` can change during execution of the
// `hooks`.
if (processing_hook === 0 && tmp_active_hooks_array !== null) {
restoreTmpHooks();
}
processing_hook = false;
}


Expand Down
23 changes: 23 additions & 0 deletions test/async-hooks/test-disable-in-init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const async_hooks = require('async_hooks');
const fs = require('fs');

let nestedCall = false;

async_hooks.createHook({
init: common.mustCall(function(id, type) {
nestedHook.disable();
if (!nestedCall) {
nestedCall = true;
fs.access(__filename, common.mustCall());
}
}, 2)
}).enable();

const nestedHook = async_hooks.createHook({
init: common.mustCall(2)
}).enable();

fs.access(__filename, common.mustCall());
22 changes: 22 additions & 0 deletions test/async-hooks/test-enable-in-init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

const common = require('../common');
const async_hooks = require('async_hooks');
const fs = require('fs');

const nestedHook = async_hooks.createHook({
init: common.mustNotCall()
});
let nestedCall = false;

async_hooks.createHook({
init: common.mustCall(function(id, type) {
nestedHook.enable();
if (!nestedCall) {
nestedCall = true;
fs.access(__filename, common.mustCall());
}
}, 2)
}).enable();

fs.access(__filename, common.mustCall());
19 changes: 19 additions & 0 deletions test/parallel/test-async-hooks-enable-recursive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';

const common = require('../common');
const async_hooks = require('async_hooks');
const fs = require('fs');

const nestedHook = async_hooks.createHook({
init: common.mustCall()
});

async_hooks.createHook({
init: common.mustCall((id, type) => {
nestedHook.enable();
}, 2)
}).enable();

fs.access(__filename, common.mustCall(() => {
fs.access(__filename, common.mustCall());
}));