src,http: fix uncaughtException miss in http

trevnorris · trevnorris · commit 3521b052b175 · 2016-03-08T10:45:58.000-07:00
In AsyncWrap::MakeCallback always return empty handle if there is an error. In the future this should change to return a v8::MaybeLocal, but that major change will have to wait for v6.x, and these changes are meant to be backported to v4.x. The HTTParser call to AsyncWrap::MakeCallback failed because it expected a thrown call to return an empty handle. In node::MakeCallback return an empty handle if the call is in_makecallback(), otherwise return v8::Undefined() as usual to preserve backwards compatibility. Fixes: #5555 PR-URL: #5591 Reviewed-By: Julien Gilli <jgilli@nodejs.org>
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
@@ -193,7 +193,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     if (has_domain) {
       domain = domain_v.As<Object>();
       if (domain->Get(env()->disposed_string())->IsTrue())
-        return Undefined(env()->isolate());
+        return Local<Value>();
     }
   }
 
@@ -220,7 +220,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }
 
   if (ret.IsEmpty()) {
-    return Undefined(env()->isolate());
+    return ret;
   }
 
   if (has_domain) {
@@ -249,7 +249,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }
 
   if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
-    return Undefined(env()->isolate());
+    return Local<Value>();
   }
 
   return ret;
diff --git a/src/node.cc b/src/node.cc
@@ -1198,7 +1198,11 @@ Local<Value> MakeCallback(Environment* env,
   }
 
   if (ret.IsEmpty()) {
-    return Undefined(env->isolate());
+    if (callback_scope.in_makecallback())
+      return ret;
+    // NOTE: Undefined() is returned here for backwards compatibility.
+    else
+      return Undefined(env->isolate());
   }
 
   if (has_domain) {
diff --git a/test/parallel/test-http-catch-uncaughtexception.js b/test/parallel/test-http-catch-uncaughtexception.js
@@ -0,0 +1,23 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const uncaughtCallback = common.mustCall(function(er) {
+  assert.equal(er.message, 'get did fail');
+});
+
+process.on('uncaughtException', uncaughtCallback);
+
+const server = http.createServer(function(req, res) {
+  res.writeHead(200, { 'Content-Type': 'text/plain' });
+  res.end('bye');
+}).listen(common.PORT, function() {
+  http.get({ port: common.PORT }, function(res) {
+    res.resume();
+    throw new Error('get did fail');
+  }).on('close', function() {
+    server.close();
+  });
+});

Original file line number	Diff line number	Diff line change
`@@ -193,7 +193,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,`
`193`	`193`	`if (has_domain) {`
`194`	`194`	`domain = domain_v.As<Object>();`
`195`	`195`	`if (domain->Get(env()->disposed_string())->IsTrue())`
`196`		`- return Undefined(env()->isolate());`
	`196`	`+ return Local<Value>();`
`197`	`197`	`}`
`198`	`198`	`}`
`199`	`199`
`@@ -220,7 +220,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,`
`220`	`220`	`}`
`221`	`221`
`222`	`222`	`if (ret.IsEmpty()) {`
`223`		`- return Undefined(env()->isolate());`
	`223`	`+ return ret;`
`224`	`224`	`}`
`225`	`225`
`226`	`226`	`if (has_domain) {`
`@@ -249,7 +249,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,`
`249`	`249`	`}`
`250`	`250`
`251`	`251`	`if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {`
`252`		`- return Undefined(env()->isolate());`
	`252`	`+ return Local<Value>();`
`253`	`253`	`}`
`254`	`254`
`255`	`255`	`return ret;`
Original file line number	Diff line number	Diff line change
`@@ -1198,7 +1198,11 @@ Local<Value> MakeCallback(Environment* env,`
`1198`	`1198`	`}`
`1199`	`1199`
`1200`	`1200`	`if (ret.IsEmpty()) {`
`1201`		`- return Undefined(env->isolate());`
	`1201`	`+ if (callback_scope.in_makecallback())`
	`1202`	`+ return ret;`
	`1203`	`+ // NOTE: Undefined() is returned here for backwards compatibility.`
	`1204`	`+ else`
	`1205`	`+ return Undefined(env->isolate());`
`1202`	`1206`	`}`
`1203`	`1207`
`1204`	`1208`	`if (has_domain) {`