Fix nexttick exception

[RobinQu]

Exceptions should be thrown and process shall exit if there is no listener for uncaughtException event.

I modified the way we run post and destroy hooks after an exception happens during next tick.

[AndreasMadsen]

Good catch, but this is not the correct fix. The timing is wrong and we can't change the throw origin. Just add a check for emitter.listenerCount('uncaughtException') > 0 in the didThrow part.

[RobinQu]

I understand there should not be an explicit catch part. But the reason to invoke post and destroy hooks in uncaughtException handler is beyond me.

[AndreasMadsen]

Look at the timing test, you made the following changes:

@@ -40,7 +40,7 @@ process.once('exit', function () {
    assert.strictEqual(throwFlag, true);
    assert.deepEqual(eventOrder, [
     'init#-1 NextTickWrap', 'pre#-1',
-    'callback', 'exception',
-    'post#-1', 'destroy#-1'
+    'callback',
+    'post#-1', 'destroy#-1', 'exception'
    ]);
  });

The timing you implemented is not correct, because the exception needs to happen before post. This is a consequence of how AsyncWrap is implemented in C++: https://github.com/nodejs/node/blob/master/src/async-wrap.cc#L238

To implement this in JS I delay post and destroy to after all the users uncaughtException event handlers have fired. This was done by using .on(ce), because the EventEmitter timing is FIFO.

[RobinQu]

I made some improvements that passed the original test.

[RobinQu]

BTW. I suppose the moment the proposal changes in node-eps land in a stable release of node, async-hook should also be changed dramatically?

[AndreasMadsen]

I would like to not mess with _fatalException. If the callback thows and there is no uncaughtException handler, I would think that the process exists before the post and destroy hooks can be called, correct? In that case you don't have to do anything.

[RobinQu]

Well, the definition is quite ambiguous.

https://github.com/nodejs/node-eps/pull/18/files#diff-6a166510b2cfd0122a8e38a6efe7986eR139

[RobinQu]

@AndreasMadsen I am stuck in a problem relating to the HTTPParser. I understand this is not a problem about async-hook. But I hope you can share some opinions about this failing test case: https://github.com/RobinQu/async-zone/issues/1

[AndreasMadsen]

I have tested it using the current AsyncWrap implementation. As expected the post and destroy hooks are not called. I think we should stick to the current behaviour.

---

The test case:

'use strict';

const asyncHook = require('./');
const assert = require('assert');
const fs = require('fs');

asyncHook.addHooks({
  init: function (uid, handle) {
    process._rawDebug(`init#${uid} ${handle.constructor.name}`);
  },
  pre: function (uid) {
    process._rawDebug(`pre#${uid}`);
  },
  post: function (uid, handle, didThrow) {
    process._rawDebug(`post#${uid}`);
  },
  destroy: function (uid) {
    process._rawDebug(`destroy#${uid}`);
  }
});

asyncHook.enable();

/*
process.once('uncaughtException', function () {
  process._rawDebug('exception');
});
*/

fs.access(__filename, function () {
  process._rawDebug('callback');
  throw new Error('error');
});

asyncHook.disable();

without uncaughtException:

init#1 FSReqWrap
pre#1
callback
/Users/Andreas/Sites/node_modules/async-hook/test-fsaccess-didthrow.js:31
  throw new Error('error');
  ^

Error: error
    at FSReqWrap.oncomplete (fs.js:123:15)

with uncaughtException

init#1 FSReqWrap
pre#1
callback
exception
post#1
destroy#1

[RobinQu]

I got it....

[AndreasMadsen]

Great, do you want to fix timers too?

[RobinQu]

@AndreasMadsen Ok, I will take some time to fix it.

BTW, my colleagues and I are attending Velocity conference held in NYC around Sept. 22th. I am wondering if we could meet some guys of Node.js team members in NYC. Do you guys have some regular meetings in NYC?

[AndreasMadsen]

Thanks. I will make the final review later. Most likely I will just fix the nits myself, if that is ok.

BTW, my colleagues and I are attending Velocity conference held in NYC around Sept. 22th. I am wondering if we could meet some guys of Node.js team members in NYC.

No idea, I live in Denmark. You should should ask on Twitter and https://github.com/nodejs/diagnostics/.

[RobinQu]

OK. That's fine.

[AndreasMadsen]

Thanks. Landed in 129af3d9f4961ecb1599c1232988fe87aece61ee and c4525789d04a26683bf2d54e44a33bdc5f730dee. Additional tests in 3cc56077758c3a29a86d8329e54e9684c7ce81a4 and 5d7695a1ebefd83f8aaf36c745938c416ef0d525.