async_hooks: fix nested hooks mutation #14143

AndreasMadsen · 2017-07-09T12:17:46Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

In some cases restoreTmpHooks is called too early, this causes active_hooks_array to change during execution of the init hooks.

This depends on #14054

AndreasMadsen · 2017-07-10T12:07:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9058/

AndreasMadsen · 2017-07-10T13:05:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9059/

addaleax · 2017-07-10T14:13:26Z

test/parallel/test-async-hooks-enable-recursive.js

+
+const common = require('../common');
+const async_hooks = require('async_hooks');
+const crypto = require('crypto');


You kept crypto here, was that intentional? If so, it probably needs a guard for skipping the test if appropriate

That is from the blocking PR, I will rebase once that is fixed.

refack

-0.1

refack · 2017-07-10T17:01:41Z

lib/async_hooks.js

 }

 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 {


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))

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.

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

refack

Just a 2 nits

refack · 2017-07-10T17:40:01Z

lib/async_hooks.js

@@ -359,9 +354,9 @@ function emitHookFactory(symbol, name) {
    } catch (e) {
      fatalError(e);
    }
-    processing_hook = false;
+    processing_hook -= 1;


nit: Since we are going with depth counting, IMHO the decrement should be in a finally clause

I guess, but there is no way to escape fatalError so the code will never be executed.

The try catch finally order is:

try { throw new Error(); } catch (e) { console.log('catch'); } finally { console.log('finally'); }

outputs catch finally.

I guess, but there is no way to escape fatalError so the code will never be executed.

At the moment... If at some point we'll enable a fatalError trap this will break. Better to be explicit about these.
Reference/depth counting only works if it's ACID

refack · 2017-07-10T17:40:23Z

lib/async_hooks.js

@@ -441,7 +436,18 @@ function init(asyncId, type, triggerAsyncId, resource) {
  } catch (e) {
    fatalError(e);
  }
-  processing_hook = false;
+  processing_hook -= 1;


This fixes an error that could occure by nesting async_hooks calls

In some cases restoreTmpHooks is called too early, this causes active_hooks_array to change during execution of the init hooks.

for when node is compiled without crypto support

AndreasMadsen · 2017-07-12T16:25:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9092/

trevnorris

Thanks much. Especially for the additional code comment in init(). Have a non-blocking nit to change the processing_hooks code comment to explain that it's a depth counter.

trevnorris · 2017-07-12T20:26:49Z

lib/async_hooks.js

@@ -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;


Possibly change comment above from simply

Track whether a hook [...]

to something like

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

AndreasMadsen · 2017-07-13T07:53:18Z

Fixed the nit. I had to use some artistic freedom to fit in the "prevent nested calls" part. But the meaning should be the same.

CI: https://ci.nodejs.org/job/node-test-pull-request/9106/

AndreasMadsen · 2017-07-13T10:02:50Z

landed in f94fd0c

trevnorris · 2017-07-13T21:53:26Z

@AndreasMadsen Awesome. Thanks for adding the comment changes.

In some cases restoreTmpHooks is called too early, this causes active_hooks_array to change during execution of the init hooks. PR-URL: #14143 Ref: #14054 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

AndreasMadsen added the blocked PRs that are blocked by other issues or PRs. label Jul 9, 2017

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 9, 2017

AndreasMadsen mentioned this pull request Jul 9, 2017

async_hooks: move statement #14054

Closed

4 tasks

addaleax approved these changes Jul 10, 2017

View reviewed changes

refack reviewed Jul 10, 2017

View reviewed changes

refack approved these changes Jul 10, 2017

View reviewed changes

BridgeAR and others added 6 commits July 12, 2017 18:23

async_hooks: move restoreTmpHooks call to init

a4cca58

This fixes an error that could occure by nesting async_hooks calls

fixup: use fs instead of crypto

19890ce

async_hooks: fix nested hooks mutation

3e5d7ba

In some cases restoreTmpHooks is called too early, this causes active_hooks_array to change during execution of the init hooks.

[squash] lint

65d4760

[squash] don't use crypto

d04802b

for when node is compiled without crypto support

[squash] use finally block

0b80a31

trevnorris approved these changes Jul 12, 2017

View reviewed changes

[squash] update comment

cbd1f78

AndreasMadsen closed this Jul 13, 2017

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: fix nested hooks mutation #14143

async_hooks: fix nested hooks mutation #14143

AndreasMadsen commented Jul 9, 2017

AndreasMadsen commented Jul 10, 2017

AndreasMadsen commented Jul 10, 2017

addaleax Jul 10, 2017

AndreasMadsen Jul 10, 2017

refack left a comment

refack Jul 10, 2017

AndreasMadsen Jul 10, 2017 •

edited

Loading

refack Jul 10, 2017

refack left a comment

refack Jul 10, 2017

AndreasMadsen Jul 10, 2017 •

edited

Loading

refack Jul 10, 2017

refack Jul 10, 2017

AndreasMadsen commented Jul 12, 2017

trevnorris left a comment

trevnorris Jul 12, 2017

AndreasMadsen commented Jul 13, 2017

AndreasMadsen commented Jul 13, 2017

trevnorris commented Jul 13, 2017

async_hooks: fix nested hooks mutation #14143

async_hooks: fix nested hooks mutation #14143

Conversation

AndreasMadsen commented Jul 9, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jul 10, 2017

AndreasMadsen commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 12, 2017

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 13, 2017

AndreasMadsen commented Jul 13, 2017

trevnorris commented Jul 13, 2017

AndreasMadsen Jul 10, 2017 •

edited

Loading

AndreasMadsen Jul 10, 2017 •

edited

Loading