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.

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

async_hooks initial implementation #8531

Closed
wants to merge 40 commits into from

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Sep 14, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

async_wrap, async_hooks

Description of change

This is the initial implementation of the async_hooks API as outlined in nodejs/node-eps#18. There was a more extensive API initially proposed, but after completing all those features I found it had substantial performance impact. So I limited the scope of the proposal (the EP has just been updated to reflect those changes) and implemented them. I left the feature complete implementation in its own commit, and stripped it down in a following commit. So that reviewers could see how it was being done and possibly have some suggestions about how to improve that performance hit.

The biggest hit comes from tracking process.nextTick() calls. Here's a minimal http server with hooks:

const print = process._rawDebug;

require('async_hooks').createHook({
  init: (id, type) => print('init:   ', id, type),
  before: (id) =>     print('before: ', id),
  after: (id) =>      print('after:  ', id),
  destroy: (id) =>    print('destroy:', id),
}).enable();

http.createServer((req, res) => {
  res.end('bye');
}).listen(8080);

And here is the async call stack for a single request completion:

init:    21 TCPWRAP
before:  2
init:    22 TickObject
after:   2
before:  22
after:   22
destroy: 22
before:  6
init:    23 TIMERWRAP
init:    24 TickObject
init:    25 TickObject
after:   6
before:  6
init:    26 TickObject
after:   6
before:  6
after:   6
before:  24
init:    27 TickObject
after:   24
destroy: 24
before:  25
after:   25
destroy: 25
before:  26
init:    28 TickObject
after:   26
destroy: 26
before:  27
init:    29 TickObject
init:    30 TickObject
init:    31 TickObject
after:   27
destroy: 27
before:  28
after:   28
destroy: 28
before:  29
after:   29
destroy: 29
before:  30
after:   30
destroy: 30
before:  31
after:   31
destroy: 31
before:  21
init:    32 TickObject
init:    33 TickObject
after:   21
before:  32
after:   32
destroy: 32
before:  33
after:   33
destroy: 33
before:  21
after:   21
destroy: 21
before:  23
after:   23
destroy: 23

As you can see, the TickObject calls dominate the stack. So even doing something as minimal as generating a new id for each TickObject can be seen when stressing out a process.

The biggest functionality hit that was taken is that the hooks don't propagate with the call stack anymore. Instead once you run disable() they are removed from the global pool of hooks to process, and won't be run again until the user runs enable().

Would appreciate any feedback on how to reintroduce what was removed. Though even stripped down I'm seeing a few percent performance drop w/o using any callbacks. My goal was to have zero (or maybe better said is indistinguishable) overhead if the API wasn't being used. IMO that should be the first thing addressed.

Known Issues

This PR still has some bugs to fix. Here is a list:

  • HTTPParser doesn't propagate parent id properly
  • Reused resources aren't reassigned a new UID
  • Have potential for leak if references are made to handles that require GC for destroy() call

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. async_hooks Issues and PRs related to the async hooks subsystem. labels Sep 14, 2016
@Fishrock123
Copy link
Contributor

@Fishrock123
Copy link
Contributor

cc @nodejs/ctc & @AndreasMadsen

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Looking very good so far. Left a couple minor comments but otherwise this is +1. Will have to do a more detailed review later on.

const async_hook_fields = async_wrap.async_hook_fields;
// Array of all AsyncHooks that will be iterated whenever an async event fires.
const active_hooks_array = [];
const isBE = require('os').endianness() === 'BE';
Copy link
Member

Choose a reason for hiding this comment

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

Ugh. Can we please consider making this a constant in os.constants rather than a method?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a method for a long time, I see little reason to get rid of it.

}

if (!Number.isSafeInteger(id) || id <= 0) {
throw new RangeError('id must be an unsigned integer');
Copy link
Member

Choose a reason for hiding this comment

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

Given that there's not a clear concept of unsigned integers in js, perhaps this should say that it must be a non-negative integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you specifically referring to the bit that's interpreted differently depending on whether it's unsigned or not? I am using the definition of

capable of representing only non-negative integers

I'll change the wording if you like, but personally don't think it's necessary.

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

Looks really good, but it's very tough to review.

I think most issues are documented in the TODOs, I found only one bug. Besides that:

  • I think you should create an async_hooks binding. Having the current_id, async_id, etc. counters in async_wrap seams to only be for historic reasons.
  • It appears that the C++ Embedder API hasn't been implemented.
  • Also if it helps, you herby have permission to steal tests from my async_hook module.

}

if (!Number.isSafeInteger(id) || id <= 0) {
throw new RangeError('id must be an unsigned integer');
Copy link
Member

Choose a reason for hiding this comment

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

Just in case the id do get too high it might be a good idea to say it must be an unsigned safe integer. Otherwise it will be confusing when 9007199254740992 throw an error.

// use kActiveHooks for the for loop length so adding hooks to the array
// won't be executed.
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][init_symbol] === 'function') {
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016

Choose a reason for hiding this comment

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

I haven't checked, but I would assume that checking active_hooks_array[i][init_symbol] !== undefined is faster than looking up the type.



function emitBefore(id) {
if (active_hooks_array[kBefore] === 0) {
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016

Choose a reason for hiding this comment

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

I understand there are performance reasons for not checking with isSafeInteger. But I find it just as likely that I would pass a wrong argument to emitBefore as I would pass a wrong argument to emitInit.



// TODO(trevnorris): Not married to this passing the previoud id thing. Is
// convenient but is also a little ugly.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, what is prevId about? It is not even used in emitAfter. I think you removed it from the documentation too.

async_hook_fields[kAfter] -= +!!this[after_symbol];
async_hook_fields[kDestroy] -= +!!this[destroy_symbol];
async_hook_fields[kActiveHooks]--;
active_hooks_array.splice(index, 1);
Copy link
Member

Choose a reason for hiding this comment

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

We should document that hook.disable().enable() can change the order.

lib/timers.js Outdated
// TODO(trevnorris): There is a performance hit of always needing to place
// the current TimerWrap id on the hooks stack. We know that TimerWrap is
// only a mechanism to fire these callbacks, so instead manually set the
// correct current id field from here.
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016

Choose a reason for hiding this comment

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

hooks stack

Can you elaborate on what the hooks stack is.

//
// The XOR check in case both can't be set.
//CHECK_NE(env->current_async_id() > 0, parent != nullptr);
/* debug:start */
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016

Choose a reason for hiding this comment

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

Just want to make sure you didn't forget this.

return Local<Value>();
}

if (async_hooks->fields()[Environment::AsyncHooks::kBefore] > 0) {
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016

Choose a reason for hiding this comment

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

Should be Environment::AsyncHooks::kAfter

kinda happy I found an error, halfway through I was starting to think it was perfect :)

Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
Local<Value> vals[] = { uid, did_throw };
Local<Value> ret_v;
if (!ret.ToLocal(&ret_v)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so strong on my C++ V8, what does this mean? I assume it is similar to:

if (ret.IsEmpty()) {
  return ret;
}

but before that was checked/called after the post hook.

@@ -1206,34 +1198,8 @@ Local<Value> MakeCallback(Environment* env,
}
}

if (ran_init_callback && !pre_fn.IsEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

How is before and after called now. The propused API is that MakeCallback(..., async_id, ...) will call before and after but this doesn't appear to do that.

lib/timers.js Outdated
@@ -156,6 +160,7 @@ function TimersList(msecs, unrefed) {
function listOnTimeout() {
var list = this._list;
var msecs = list.msecs;
var prev_id = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just define this in the while where the code runs?

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Seems good, left some timers review.

(Hopefully I'm using this new review thing correctly?)

lib/timers.js Outdated
@@ -455,6 +480,9 @@ function Timeout(after) {
this._idleStart = null;
this._onTimeout = null;
this._repeat = null;
this._asyncId = async_hooks.newUid();
if (async_hook_fields[kInit])
async_hooks.emitInit(this._asyncId, this, 'Timeout');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be in insert() I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: some sort of ID retention needs to be done for intervals...

(And maybe even timers that are re-inserted...)

lib/timers.js Outdated
@@ -506,6 +534,8 @@ Timeout.prototype.close = function() {
} else {
unenroll(this);
}
if (this._asyncId && async_hook_fields[kDestroy])
async_hooks.emitDestroy(this._asyncId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in unenroll().

lib/timers.js Outdated
@@ -515,7 +545,7 @@ var immediateQueue = L.create();

function processImmediate() {
const queue = immediateQueue;
var domain, immediate;
var domain, immediate, prev_id = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same loop/variable comment as in listOnTimeout()

@@ -283,6 +282,12 @@
if (!caught)
caught = process.emit('uncaughtException', er);

// After this call is made the stack is gone.
// TODO(trevnorris): Do we want to run all after callbacks here to let
// them know an error has occurred? Since it may be a little confusing
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still valid since after() no longer has a didThrow?

@joshgav
Copy link
Contributor

joshgav commented Sep 16, 2016

/cc @nodejs/diagnostics @digitalinfinity

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Trying to get dprof running properly on this. Progress is good, but representing timers properly with this is... difficult.

I think destory() may need to have an extra parameter for resources that may be reused in the future.

For example, someone may unenroll a non-repeating timer, keep it alive, and re-enroll it later.

A quicker example of that would be any timer that gets its duration updated.

lib/timers.js Outdated
if (timer._asyncId) {
async_hooks.setCurrentId(prev_id);
if (async_hook_fields[kAfter])
async_hooks.emitAfter(timer._asyncId);
Copy link
Contributor

Choose a reason for hiding this comment

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

also needs roughly this:

if (!timer._repeat && async_hook_fields[kDestroy])
        async_hooks.emitDestroy(timer._asyncId);

lib/timers.js Outdated
@@ -455,6 +480,9 @@ function Timeout(after) {
this._idleStart = null;
this._onTimeout = null;
this._repeat = null;
this._asyncId = async_hooks.newUid();
if (async_hook_fields[kInit])
async_hooks.emitInit(this._asyncId, this, 'Timeout');
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: some sort of ID retention needs to be done for intervals...

(And maybe even timers that are re-inserted...)

@Qard
Copy link
Member

Qard commented Sep 18, 2016

@Fishrock123 Do you think it'd be more generically solvable with this? nodejs/node-eps#18 (comment)

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 18, 2016

@Qard hmmmm, could be. Wouldn't you also need an unqueue or finish then?

That could solve the TIMERWRAP - Timeout disparity though. Edit2: Maybe?

For example, take interval.dprof.json and look at the rendered version by:

npm i -g dprof
cat interval.dprof.json | dprof

(Should work with latest published dprof I think.)

Edit: the code is this, for reference:

setInterval(function timeout() {
  console.log('#')
}, 100)

@Qard
Copy link
Member

Qard commented Sep 18, 2016

Maybe something like this for cancelables:

create -- resource created
- queue -- request queued
    - before -- request reaches callback
       - after -- request completes callback
- queue -- request queued
    - unqueue -- request cancelled
destroy -- resource cleaned up

lib/timers.js Outdated
async_hooks.setCurrentId(prev_id);
if (async_hook_fields[kAfter])
async_hooks.emitAfter(immediate._asyncId);
}
Copy link
Contributor

@Fishrock123 Fishrock123 Sep 18, 2016

Choose a reason for hiding this comment

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

also needs

if (immediate._asyncId && async_hook_fields[kDestroy]) {
    async_hooks.emitDestroy(immediate._asyncId);
  }

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 18, 2016

Still needs even more timers fixes. A diff will follow this.

(From AndreasMadsen/dprof#17 (comment))

Take, for example, this.

The handles with the line in-between are actually the same, the timeout was created and the updated three times, from different places, causing re-insertion.

timers-async_hooks

UIDs at the current time (with patches for timers): 12, 17, 20, then 26.

This was taken using a WIP dprof using the following patches to get roughly correct timers behavior but without conceptual same-ID re-use patching.

diff --git a/lib/timers.js b/lib/timers.js
index 6ead9c2..3194ab4 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -145,6 +145,15 @@ function insert(item, unrefed) {
     list._timer[kOnTimeout] = listOnTimeout;
   }

+  if (unrefed === true) item._timerUnref = true;
+
+  if (!item._asyncId) {
+    // No UID, assign a new one.
+    item._asyncId = async_hooks.newUid();
+    if (async_hook_fields[kInit])
+      async_hooks.emitInit(item._asyncId, item, 'Timeout');
+  }
+
   L.append(list, item);
   assert(!L.isEmpty(list)); // list is not empty
 }
@@ -188,7 +197,14 @@ function listOnTimeout() {
     L.remove(timer);
     assert(timer !== L.peek(list));

-    if (!timer._onTimeout) continue;
+    if (!timer._onTimeout) {
+      if (timer._asyncId && async_hook_fields[kDestroy]) {
+        async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
+
+      continue;
+    }

     var domain = timer.domain;
     if (domain) {
@@ -224,6 +240,10 @@ function listOnTimeout() {
       async_hooks.setCurrentId(prev_id);
       if (async_hook_fields[kAfter])
         async_hooks.emitAfter(timer._asyncId);
+      if (!timer._repeat && async_hook_fields[kDestroy]) {
+        async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
     }

     if (domain)
@@ -300,6 +320,12 @@ function reuse(item) {

 // Remove a timer. Cancels the timeout and resets the relevant timer properties.
 const unenroll = exports.unenroll = function(item) {
+  if (item._asyncId) {
+    if (async_hook_fields[kDestroy])
+      async_hooks.emitDestroy(item._asyncId);
+    item._asyncId = 0;
+  }
+
   var handle = reuse(item);
   if (handle) {
     debug('unenroll: list empty');
@@ -480,9 +506,7 @@ function Timeout(after) {
   this._idleStart = null;
   this._onTimeout = null;
   this._repeat = null;
-  this._asyncId = async_hooks.newUid();
-  if (async_hook_fields[kInit])
-    async_hooks.emitInit(this._asyncId, this, 'Timeout');
+  this._asyncId = 0;
 }


@@ -534,8 +558,6 @@ Timeout.prototype.close = function() {
   } else {
     unenroll(this);
   }
-  if (this._asyncId && async_hook_fields[kDestroy])
-    async_hooks.emitDestroy(this._asyncId);
   return this;
 };

@@ -575,6 +597,10 @@ function processImmediate() {
         async_hooks.emitAfter(immediate._asyncId);
     }

+    if (immediate._asyncId && async_hook_fields[kDestroy]) {
+      async_hooks.emitDestroy(immediate._asyncId);
+    }
+
     if (domain)
       domain.exit();
   }

@Fishrock123
Copy link
Contributor

Perhaps it is better to just re-use the same UID and forget about re-insertion pre- timeout/unenroll.

That is, only destroy and re-insert after timeout or removal, not on updating the timeout duration or start (re-insert.)

e.g. this patch on the last one:

diff --git a/lib/timers.js b/lib/timers.js
index 9f397cd..3194ab4 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -152,13 +152,6 @@ function insert(item, unrefed) {
     item._asyncId = async_hooks.newUid();
     if (async_hook_fields[kInit])
       async_hooks.emitInit(item._asyncId, item, 'Timeout');
-  } else if (!item._repeat) {
-    // UID but no repeat, emit Destroy on the existing, then create a new one.
-    if (async_hook_fields[kDestroy])
-      async_hooks.emitDestroy(item._asyncId);
-    item._asyncId = async_hooks.newUid();
-    if (async_hook_fields[kInit])
-      async_hooks.emitInit(item._asyncId, item, 'Timeout');
   }

   L.append(list, item);
@@ -205,8 +198,10 @@ function listOnTimeout() {
     assert(timer !== L.peek(list));

     if (!timer._onTimeout) {
-      if (timer._asyncId && async_hook_fields[kDestroy])
+      if (timer._asyncId && async_hook_fields[kDestroy]) {
         async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }

       continue;
     }
@@ -245,8 +240,10 @@ function listOnTimeout() {
       async_hooks.setCurrentId(prev_id);
       if (async_hook_fields[kAfter])
         async_hooks.emitAfter(timer._asyncId);
-      if (!timer._repeat && async_hook_fields[kDestroy])
+      if (!timer._repeat && async_hook_fields[kDestroy]) {
         async_hooks.emitDestroy(timer._asyncId);
+        timer._asyncId = 0
+      }
     }

     if (domain)

Gives this output: (Same handle circled in orange, UID 12.)

timers-maybe-better-async_wrap

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 19, 2016

It may be worthwhile to have timers use their own unique _asyncId property though, since timers can be made off of other objects that could have an _asyncId it could get very confusing...

e.g. _timerAsyncId

Edit: or we could just use symbol-hidden properties.

@trevnorris
Copy link
Contributor Author

@Fishrock123

Perhaps it is better to just re-use the same UID and forget about re-insertion pre- timeout/unenroll.

That is, only destroy and re-insert after timeout or removal, not on updating the timeout duration or start (re-insert.)

agreed.

It may be worthwhile to have timers use their own unique _asyncId property though, since timers can be made off of other objects that could have an _asyncId it could get very confusing...

e.g. _timerAsyncId

to clarify, you mean to use a different property name and not to have their own unique id mechanism? that works for me.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 20, 2016

to clarify, you mean to use a different property name and not to have their own unique id mechanism? that works for me.

I mean a different property name for the ID specific to timers so that it does not conflict because any object could be a timer. (yes, I think...?)

@joshgav joshgav removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Feb 23, 2017
@trevnorris
Copy link
Contributor Author

Superseded by #11883. Closing.

@trevnorris trevnorris closed this Mar 16, 2017
@bajtos
Copy link
Contributor

bajtos commented Mar 27, 2017

Superseded by #11883. Closing.

@trevnorris would you mind updating the checklist in nodejs/diagnostics#29 with the new PR number?

@AndreasMadsen
Copy link
Member

@bajtos Thanks, I've just done that.

addaleax pushed a commit that referenced this pull request May 10, 2017
Also add checks in lib/tty.js and tests.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
AsyncWrap will be going through many changes, and the old API will no
longer be used. So remove those tests that will no longer be useful.
They may be added back later using the new API, once fully implemented.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
The number of ids is limited to 2^53-1 regardless of whether an int64_t
is used or not because JS is limited to a double. So to make conversion
simpler, track ids internally as a double. This will also make life
simpler when this is eventually exposed to JS via a Float64Array.

Rename AsyncWrap::get_uid() to AsyncWrap::get_id().

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
RandomBytes and PBKDF2 were using the same "generic" ObjectTemplate for
construction. Instead create one for each that is properly named.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Instead of wrapping several providers into PROVIDER_CRYPTO, have them
all be named after their class. Rename other providers to also match
their class names. With the exception of Parser. Which is actually
HTTPParser.

Add PROVIDER_LENGTH to make better checks in WrapperInfo().

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Allow handles to retrieve their own uid's by adding a new method on the
FunctionTemplates. Implementation of these into all other classes will
come in a future commit.

Add the method AsyncWrap::GetAsyncId() to all inheriting class objects
so the uid of the handle can be retrieved from JS.

In all applicable locations, run ClearWrap() on the object holding the
pointer so that it never points to invalid memory and make sure Wrap()
is always run so the class pointer is correctly attached to the object
and can be retrieved so GetAsyncId() can be run.

In many places a class instance was not removing its own pointer from
object() in the destructor. This left an invalid pointer in the JS
object that could cause the application to segfault under certain
conditions.

Remove ClearWrap() from ReqWrap for continuity. The ReqWrap constructor
was not the one to call Wrap(), so it shouldn't be the one to call
ClearWrap().

Wrap() has been added to all constructors that inherit from AsyncWrap.
Normally it's the child most class. Except in the case of HandleWrap.
Which must be the constructor that runs Wrap() because the class pointer
is retrieved for certain calls and because other child classes have
multiple inheritance to pointer to the HandleWrap needs to be stored.

ClearWrap() has been placed in all FunctionTemplate constructors so that
no random values are returned when running getAsyncId(). ClearWrap() has
also been placed in all class destructors, except in those that use
MakeWeak() because the destructor will run during GC. Making the
object() inaccessible.

It could be simplified to where AsyncWrap sets the internal pointer,
then if an inheriting class needs one of it's own it could set it again.
But the inverse would need to be true also, where AsyncWrap then also
runs ClearWeak. Unforunately because some of the handles are cleaned up
during GC that's impossible. Also in the case of ReqWrap it runs Reset()
in the destructor, making the object() inaccessible. Meaning,
ClearWrap() must be run by the class that runs Wrap(). There's currently
no generalized way of taking care of this across all instances of
AsyncWrap.

I'd prefer that there be checks in there for these things, but haven't
found a way to place them that wouldn't be just as unreliable.

Add test that checks all resources that can run getAsyncId(). Would like
a way to enforce that any new classes that can also run getAsyncId() are
tested, but don't have one.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Changes in the native code for the upcoming async_hooks module. These
have been separated to help with review and testing.

Changes include:

* Introduce an async id stack that tracks recursive calls into async
  execution contexts. For performance reasons the id stack is held as a
  double* and assigned to a Float64Array. If the stack grows too large
  it is then placed in it's own stack and replaced with a new double*.
  This should accommodate arbitrarily large stacks.

  I'm not especially happy with the complexity involved with this async
  id stack, but it's also the fastest and most full proof way of
  handling it that I have found.

* Add helper functions in Environment and AsyncWrap to work with the
  async id stack.

* Add AsyncWrap::Reset() to allow AsyncWrap instances that have been
  placed in a resource pool, instead of being released, to be
  reinitialized. AsyncWrap::AsyncWrap() also now uses Reset() for
  initialization.

* AsyncWrap* parent no longer needs to be passed via the constructor.

* Introduce Environment::AsyncHooks class to contain the needed native
  functionality. This includes the pointer to the async id stack, and
  array of v8::Eternal<v8::String>'s that hold the names of all
  providers, mechanisms for storing/retrieving the trigger id, etc.

* Introduce Environment::AsyncHooks::ExecScope as a way to track the
  current id and trigger id of function execution via RAII.

* If the user passes --abort-on-uncaught-exception then instead of
  throwing the application will print a stack trace and abort.

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Fill this commit messsage with more details about the change once all
changes are rebased.

* Add lib/async_hooks.js

* Add JS methods to AsyncWrap for handling the async id stack

* Introduce AsyncReset() so that JS functions can reset the id and again
  trigger the init hooks, allow AsyncWrap::Reset() to be called from JS
  via asyncReset().

* Add env variable to test additional things in test/common.js

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Implement async_hooks support in the following:

* fatalException handler
* process.nextTick
* Timers
* net/dgram/http

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
Async wrap providers tested:

- crypto.randomBytes
- crypto.pbkdf2
- fs event wrap
- fsreqwrap access
- fsreqwrap readFile
- getaddrinforeq wrap
- getnameinforeq wrap
- pipe connect wrap
- query wrap
- pipewrap
- processwrap
- shutdown wrap
- tcpwrap
- udpwrap
- send wrap
- detailed signal wrap
- statwatcher
- timerwrap via setTimeout
- timerwrap via setInterval
- for Immediate
- http parser request
- http parser response
- connection via ssl server
- tls wrap
- write wrap
- ttywrap via readstream
- ttywrap via wriream
- zctx via zlib binding deflate

Embedder API:

-  async-event tests
  - one test looks at the happy paths
  - another ensures that in cases of events emitted in an order that
  doesn't make sense, the order is enforced by async hooks throwing a
  meaningful error
  - embedder enforcement tests are split up since async hook stack
  corruption now the process
  - therefore we launch a child and check for error output of the offending code

Additional tests:

- tests that show that we can enable/disable hooks inside their lifetime
events
- tests that verify the graph of resources triggering the creation of
other resources

Test Helpers:

- init-hooks:
  - returns one collector instance
  - when created an async hook is created and the lifetime events are
  registered to call the appropriate collector functions
  - the collector also exposes `enable` and `disable` functions which call
  through to the async hook

- hook checks:
  - checks invocations of life time hooks against the actual invocations
  that were collected
  - in some cases like `destroy` a min/max range of invocations can be
  supplied since in these cases the exact number is non-deterministic

- verify graph:
  - verifies the triggerIds of specific async resources are as expected,
  i.e. the creation of resources was triggered by the resource we expect
  - includes a printGraph function to generate easily readable test
  input for verify graph
  - both functions prune TickObjects to create less brittle and easier
  to understand tests

PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this pull request May 10, 2017
PR-URL: #12892
Ref: #11883
Ref: #8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Also add checks in lib/tty.js and tests.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
AsyncWrap will be going through many changes, and the old API will no
longer be used. So remove those tests that will no longer be useful.
They may be added back later using the new API, once fully implemented.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
The number of ids is limited to 2^53-1 regardless of whether an int64_t
is used or not because JS is limited to a double. So to make conversion
simpler, track ids internally as a double. This will also make life
simpler when this is eventually exposed to JS via a Float64Array.

Rename AsyncWrap::get_uid() to AsyncWrap::get_id().

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
RandomBytes and PBKDF2 were using the same "generic" ObjectTemplate for
construction. Instead create one for each that is properly named.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Instead of wrapping several providers into PROVIDER_CRYPTO, have them
all be named after their class. Rename other providers to also match
their class names. With the exception of Parser. Which is actually
HTTPParser.

Add PROVIDER_LENGTH to make better checks in WrapperInfo().

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Allow handles to retrieve their own uid's by adding a new method on the
FunctionTemplates. Implementation of these into all other classes will
come in a future commit.

Add the method AsyncWrap::GetAsyncId() to all inheriting class objects
so the uid of the handle can be retrieved from JS.

In all applicable locations, run ClearWrap() on the object holding the
pointer so that it never points to invalid memory and make sure Wrap()
is always run so the class pointer is correctly attached to the object
and can be retrieved so GetAsyncId() can be run.

In many places a class instance was not removing its own pointer from
object() in the destructor. This left an invalid pointer in the JS
object that could cause the application to segfault under certain
conditions.

Remove ClearWrap() from ReqWrap for continuity. The ReqWrap constructor
was not the one to call Wrap(), so it shouldn't be the one to call
ClearWrap().

Wrap() has been added to all constructors that inherit from AsyncWrap.
Normally it's the child most class. Except in the case of HandleWrap.
Which must be the constructor that runs Wrap() because the class pointer
is retrieved for certain calls and because other child classes have
multiple inheritance to pointer to the HandleWrap needs to be stored.

ClearWrap() has been placed in all FunctionTemplate constructors so that
no random values are returned when running getAsyncId(). ClearWrap() has
also been placed in all class destructors, except in those that use
MakeWeak() because the destructor will run during GC. Making the
object() inaccessible.

It could be simplified to where AsyncWrap sets the internal pointer,
then if an inheriting class needs one of it's own it could set it again.
But the inverse would need to be true also, where AsyncWrap then also
runs ClearWeak. Unforunately because some of the handles are cleaned up
during GC that's impossible. Also in the case of ReqWrap it runs Reset()
in the destructor, making the object() inaccessible. Meaning,
ClearWrap() must be run by the class that runs Wrap(). There's currently
no generalized way of taking care of this across all instances of
AsyncWrap.

I'd prefer that there be checks in there for these things, but haven't
found a way to place them that wouldn't be just as unreliable.

Add test that checks all resources that can run getAsyncId(). Would like
a way to enforce that any new classes that can also run getAsyncId() are
tested, but don't have one.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Changes in the native code for the upcoming async_hooks module. These
have been separated to help with review and testing.

Changes include:

* Introduce an async id stack that tracks recursive calls into async
  execution contexts. For performance reasons the id stack is held as a
  double* and assigned to a Float64Array. If the stack grows too large
  it is then placed in it's own stack and replaced with a new double*.
  This should accommodate arbitrarily large stacks.

  I'm not especially happy with the complexity involved with this async
  id stack, but it's also the fastest and most full proof way of
  handling it that I have found.

* Add helper functions in Environment and AsyncWrap to work with the
  async id stack.

* Add AsyncWrap::Reset() to allow AsyncWrap instances that have been
  placed in a resource pool, instead of being released, to be
  reinitialized. AsyncWrap::AsyncWrap() also now uses Reset() for
  initialization.

* AsyncWrap* parent no longer needs to be passed via the constructor.

* Introduce Environment::AsyncHooks class to contain the needed native
  functionality. This includes the pointer to the async id stack, and
  array of v8::Eternal<v8::String>'s that hold the names of all
  providers, mechanisms for storing/retrieving the trigger id, etc.

* Introduce Environment::AsyncHooks::ExecScope as a way to track the
  current id and trigger id of function execution via RAII.

* If the user passes --abort-on-uncaught-exception then instead of
  throwing the application will print a stack trace and abort.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Fill this commit messsage with more details about the change once all
changes are rebased.

* Add lib/async_hooks.js

* Add JS methods to AsyncWrap for handling the async id stack

* Introduce AsyncReset() so that JS functions can reset the id and again
  trigger the init hooks, allow AsyncWrap::Reset() to be called from JS
  via asyncReset().

* Add env variable to test additional things in test/common.js

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Implement async_hooks support in the following:

* fatalException handler
* process.nextTick
* Timers
* net/dgram/http

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Async wrap providers tested:

- crypto.randomBytes
- crypto.pbkdf2
- fs event wrap
- fsreqwrap access
- fsreqwrap readFile
- getaddrinforeq wrap
- getnameinforeq wrap
- pipe connect wrap
- query wrap
- pipewrap
- processwrap
- shutdown wrap
- tcpwrap
- udpwrap
- send wrap
- detailed signal wrap
- statwatcher
- timerwrap via setTimeout
- timerwrap via setInterval
- for Immediate
- http parser request
- http parser response
- connection via ssl server
- tls wrap
- write wrap
- ttywrap via readstream
- ttywrap via wriream
- zctx via zlib binding deflate

Embedder API:

-  async-event tests
  - one test looks at the happy paths
  - another ensures that in cases of events emitted in an order that
  doesn't make sense, the order is enforced by async hooks throwing a
  meaningful error
  - embedder enforcement tests are split up since async hook stack
  corruption now the process
  - therefore we launch a child and check for error output of the offending code

Additional tests:

- tests that show that we can enable/disable hooks inside their lifetime
events
- tests that verify the graph of resources triggering the creation of
other resources

Test Helpers:

- init-hooks:
  - returns one collector instance
  - when created an async hook is created and the lifetime events are
  registered to call the appropriate collector functions
  - the collector also exposes `enable` and `disable` functions which call
  through to the async hook

- hook checks:
  - checks invocations of life time hooks against the actual invocations
  that were collected
  - in some cases like `destroy` a min/max range of invocations can be
  supplied since in these cases the exact number is non-deterministic

- verify graph:
  - verifies the triggerIds of specific async resources are as expected,
  i.e. the creation of resources was triggered by the resource we expect
  - includes a printGraph function to generate easily readable test
  input for verify graph
  - both functions prune TickObjects to create less brittle and easier
  to understand tests

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants