Commit 63356df
src,http: fix uncaughtException miss in http
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.
Ref: #7048
Fixes: #5555
PR-URL: #5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>1 parent 6f312b3 commit 63356df
3 files changed
Lines changed: 31 additions & 4 deletions
| 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 | |
|---|---|---|---|
@@ -1177,7 +1177,11 @@ Local<Value> MakeCallback(Environment* env, | |||
| 1177 | 1177 | } | |
| 1178 | 1178 | ||
| 1179 | 1179 | if (ret.IsEmpty()) { | |
| 1180 | - return Undefined(env->isolate()); | ||
| 1180 | + if (callback_scope.in_makecallback()) | ||
| 1181 | + return ret; | ||
| 1182 | + // NOTE: Undefined() is returned here for backwards compatibility. | ||
| 1183 | + else | ||
| 1184 | + return Undefined(env->isolate()); | ||
| 1181 | 1185 | } | |
| 1182 | 1186 | ||
| 1183 | 1187 | if (has_domain) { | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,23 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + const common = require('../common'); | ||
| 4 | + const assert = require('assert'); | ||
| 5 | + const http = require('http'); | ||
| 6 | + | ||
| 7 | + const uncaughtCallback = common.mustCall(function(er) { | ||
| 8 | + assert.equal(er.message, 'get did fail'); | ||
| 9 | + }); | ||
| 10 | + | ||
| 11 | + process.on('uncaughtException', uncaughtCallback); | ||
| 12 | + | ||
| 13 | + const server = http.createServer(function(req, res) { | ||
| 14 | + res.writeHead(200, { 'Content-Type': 'text/plain' }); | ||
| 15 | + res.end('bye'); | ||
| 16 | + }).listen(common.PORT, function() { | ||
| 17 | + http.get({ port: common.PORT }, function(res) { | ||
| 18 | + res.resume(); | ||
| 19 | + throw new Error('get did fail'); | ||
| 20 | + }).on('close', function() { | ||
| 21 | + server.close(); | ||
| 22 | + }); | ||
| 23 | + }); | ||
0 commit comments