uncaughtException not called with http.get()

[martinkuba]

• Version: v5.7.1
• Platform: Darwin 14.5.0 Darwin Kernel Version 14.5.0: Wed Jul 29 02:26:53 PDT 2015; root:xnu-2782.40.9~1/RELEASE_X86_64 x86_64
• Subsystem: http, errors

With the following example, I would expect uncaughtException to be triggered. It works on v4.x but not on latest v5.

var http = require('http')

process.once('uncaughtException', function(error) {
  console.log('in uncaughtException')
  console.log(error)
})

server = http.createServer(function(req, res) {
  res.writeHead(200, {'Content-Type': 'text/plain'})
  res.end('some data')
})

server.listen(8183, function () {
  http.get('http://localhost:8183', function (res) {
    console.log('got response')
    res.resume()

    // throwing error here to trigger uncaughtException
    throw new Error("some error")
  })
})

[MylesBorins]

Wrapped that one up as a test that uses common / assert.

Here's a fun bit... everything hangs on v5, but passes on v4. BUT if we introduce a timeout (as we would need for the test) everything passes.

I am going to bisect and find out what introduced this weirdness

[MylesBorins]

Git bisect is showing that the regression was created by #5419

/cc @chrisdickinson @indutny @trevnorris

edit: here is the test I used --> https://gist.github.com/TheAlphaNerd/6615a27684deb682dfe7

[misterdjules]

@trevnorris In #4507, we had talked about the fact that not having an external (TryCatch) handler on the stack in node::AsyncWrap::MakeCallback and in node::MakeCallback would still generate a message that would be handled by node::FatalException, but after double-checking it seems clear that this cannot happen, and an external (TryCatch) handler is needed on the stack to generate such a message and eventually emit uncaughtException on the process object. My apologies for suggesting otherwise.

Putting back an external verbose handler on the stack in node::AsyncWrap::MakeCallback and node::MakeCallback fixes the bug described in this issue, but it also has the problems we talked about previously in #4507: when a callback called by MakeCallback throws, the execution of the script continues even if there's no try/catch handler.

I need to improve my knowledge about V8's exception handling before being able to help figuring out a way to solve that problem without introducing this regression.

[misterdjules]

@thealphanerd

BUT if we introduce a timeout (as we would need for the test) everything passes.

I'm assuming you were referring to the following code from https://gist.github.com/TheAlphaNerd/6615a27684deb682dfe7:

setTimeout(function() {
   console.log('test');
   common.fail('the process should throw and not timeout');
   server.close();
}, common.platformTimeout(1000))

The reason why that ends up emitting an uncaughtException event is that common is not defined, and so V8 throws an exception when running the script itself, and MakeCallback is not involved. At that time, a verbose external handler is on the stack and so the message corresponding to the error is handled as expected by node::FatalException.

Replacing the first line of that gist with var common = require('../common') hangs in the same way.

[MylesBorins]

@misterdjules good eye... womp

I've updated the gist

[trevnorris]

@misterdjules I say we add the TryCatch in MakeCallback and then figure out how to deal with script execution continuing. Usage of MakeCallback this way isn't documented yet, and as far as I'm concerned could be the "intended" behavior. Either way it'll fix this issue, which is more pressing.

[misterdjules]

@trevnorris Sounds good to me.

[trevnorris]

Almost have it working. Unfortunately is breaking the following from test-http-parser.js:

  parser[kOnHeadersComplete] = function(info) {
    throw new Error('hello world');
  };

  parser.reinitialize(HTTPParser.REQUEST);

  assert.throws(function() {
    parser.execute(request, 0, request.length);
  }, Error, 'hello world');

The error is able to bubble all the way and not be caught by assert.throws(). Not sure why that is. @misterdjules have any ideas?

[trevnorris]

So it looks like SetVerbose(true) is causing the exception to immediately bubble up to FatalException and ignore the fact that it's wrapped in a try/catch in JS. Though I can't remember the consequence for not using SetVerbose(true).

[misterdjules]

@trevnorris

Almost have it working. Unfortunately is breaking the following from test-http-parser.js:

parser[kOnHeadersComplete] = function(info) {
throw new Error('hello world');
};

parser.reinitialize(HTTPParser.REQUEST);

assert.throws(function() {
parser.execute(request, 0, request.length);
}, Error, 'hello world');

The error is able to bubble all the way and not be caught by assert.throws(). Not sure why that is. @misterdjules have any ideas?

If you put back external exception handlers (TryCatch instances) in MakeCallback and still use MakeCallback in node::Parser, it means that the MakeCallback's external exception handler is above the test's JavaScript exception handler in the stack.

Therefore, that JavaScript exception handler is not the one found when unwinding the stack to find the appropriate exception handler, and instead the external exception handler is found. Because that exception handler is verbose, a message is emitted though, which is handled by FatalException and ultimately makes the process exit, giving the impression that the error bubbled up all the way to the top of the stack.

Using plain Function::Call calls instead of MakeCallback calls in node::Parser lets exception thrown in node's HTTP parser's JS code bubble up to any JavaScript exception handler.

[misterdjules]

@trevnorris

So it looks like SetVerbose(true) is causing the exception to immediately bubble up to FatalException and ignore the fact that it's wrapped in a try/catch in JS. Though I can't remember the consequence for not using SetVerbose(true).

See my previous comment above. Basically what's happening with the current 5.7.1 version is:

1. Node sets an external exception handler on the stack and runs the top-level script.
2. The top level script is done running, and node enters the libuv event loop, but the external exception handler is gone from the stack.
3. When there's something to read on a socket, StreamBase::EmitData calls MakeCallback, which doesn't set an external exception handler on the stack.
4. The callback called by MakeCallback calls the node's HTTP parser (node::Parser) which also calls MakeCallback, and doesn't set an external exception handler.
5. An exception is thrown and not caught, but there's no external exception to propagate it too, and thus V8 doesn't report the message corresponding to the exception, and FatalException is not called.

Now what if we add an exception handler back in MakeCallback and keep using MakeCallback in node::Parser? We get the following:

1. Node sets an external exception handler on the stack and runs the top-level script.
2. The top level script is done running, and node enters the libuv event loop, but the external exception handler is gone from the stack.
3. When there's something to read on a socket, StreamBase::EmitData calls MakeCallback, which does set an external exception handler on the stack.
4. The callback called by MakeCallback calls the node's HTTP parser (node::Parser) which also calls MakeCallback, and does set a new external exception handler.
5. An exception is thrown and, even if it's caught by some user's JavaScript code, the external exception handler set in 4) is above that JavaScript handler on the stack, so the user's JavaScript exception handler won't run.
6. V8 does report the message corresponding to the exception, because the external handler set in 4) is verbose.

And then finally, the following happens when we put back external exception handlers in MakeCallback and we use Function::Call in node::Parser:

1. Node sets an external exception handler on the stack and runs the top-level script.
2. The top level script is done running, and node enters the libuv event loop, but the external exception handler is gone from the stack.
3. When there's something to read on a socket, StreamBase::EmitData calls MakeCallback, which does set an external exception handler on the stack.
4. The callback called by MakeCallback calls the node's HTTP parser (node::Parser) which doesn't call MakeCallback but instead calls Function::Call, and thus does not set a new external exception handler.
5. An exception is thrown and not caught, and there's an external exception to propagate it too, but it's below the user's JavaScript exception handler on the stack in test-http-parser.js, so the user's JavaScript exception handler runs.
6. However, in the case of the repro code of this issue, V8 does report the message corresponding to the exception, because there is a verbose external exception handler (set at step 3) on the stack and there's no user JavaScript exception handler on top of it, therefore FatalException is called.

This last behavior is what fixes this issue, and test-http-parser.js.

[trevnorris]

@misterdjules I think the following is pretty much what you explained: trevnorris@8d7b346

While that patch does fix all tests, problem is it now looses calls to the pre/post callbacks of async wrap.

[trevnorris]

@misterdjules I'm not 100% sure what I'm getting at here, but what if the external exception handler could be tied to the AsyncCallbackScope so it's reenabled or whatnot every time a new scope is entered?

[misterdjules]

@trevnorris It sounds like it could work.

[trevnorris]

@misterdjules Have an initial PR up for the fix #5571. I feel this is completely a temporary fix to get the release out, and to avoid backing the entire patch out.

Will start investigating how to propagate the external exception handler with the AsyncCallbackScope, but will most likely need some help there.

[misterdjules]

@trevnorris Actually, would you mind trying the following patch:

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 11a696c..a80ba81 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -220,7 +220,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }

   if (ret.IsEmpty()) {
-    return Undefined(env()->isolate());
+    return ret;
   }

   if (has_domain) {

instead of #5571 and let me know what you think?

Basically, returning undefined would make the HTTP parser continue and call other JavaScript functions, which would clear pending exceptions. The change above seems to fix all issues we found so far.