src,http: fix uncaughtException miss in http #5591

trevnorris · 2016-03-07T21:19:08Z

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.

Follow up from #5571 where was decided patch would land on master first.

R=@misterdjules
R=@indutny
R=@thealphanerd

CI: https://ci.nodejs.org/job/node-test-pull-request/1876/

MylesBorins · 2016-03-07T21:40:16Z

/cc @nodejs/ctc @nodejs/collaborators if anyone feels up for it please do a code review!

misterdjules · 2016-03-07T22:53:17Z

src/node.cc

@@ -1198,7 +1198,10 @@ Local<Value> MakeCallback(Environment* env,
  }

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


Would it be useful to add a comment here mentioning that undefined is returned here for backward compatibility?

done, just below.

misterdjules · 2016-03-07T23:30:38Z

@trevnorris Do we want to handle both cases (being in a recursive node::MakeCallback call or not) for https://github.com/nodejs/node/blob/master/src/node.cc#L1215 and https://github.com/nodejs/node/blob/master/src/node.cc#L1174 too?

trevnorris · 2016-03-07T23:36:58Z

@misterdjules TBH I'm more concerned about making sure everything is backward compatible w/ v4. After async wrap is backported and public i'll go through and make those APIs return MaybeLocals instead and have them always return an empty handle.

misterdjules · 2016-03-07T23:42:27Z

@trevnorris

TBH I'm more concerned about making sure everything is backward compatible w/ v4. After async wrap is backported and public i'll go through and make those APIs return MaybeLocals instead and have them always return an empty handle.

Sounds good!

Another case I missed to mention is https://github.com/nodejs/node/blob/master/src/async-wrap.cc#L196. I don't have the time to create a repro for that use case, but I'd imagine it would be possible to write one that exposes a problem where a callback executed by node::Parser would run in a domain that was disposed, and the parser would consider that there was no error when executing that callback.

What do you think?

trevnorris · 2016-03-08T00:17:09Z

@misterdjules AsyncWrap::MakeCallback is completely internal. I'll change that case.

trevnorris · 2016-03-08T17:06:29Z

Everything's fixed up. Just need the sign-off.

misterdjules · 2016-03-08T17:25:52Z

@trevnorris LGTM. Thank you!

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: nodejs#5555 PR-URL: nodejs#5591 Reviewed-By: Julien Gilli <jgilli@nodejs.org>

trevnorris · 2016-03-08T17:47:31Z

Thanks much! Landed in 3521b05.

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>

Notable changes: * child_process: “send()” now accepts an options parameter (cjihrig) #5283 - Currently the only option is “keepOpen”, which keeps the underlying socket open after the message is sent. * constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts) #5463 * Fixed two regressions which originated in v5.7.0: - http: Errors inside of http client callbacks now propagate correctly (Trevor Norris) #5591 - path: Fixed normalization of absolute paths (Evan Lucas) #5589 * repl: “start()” no longer requires an options parameter (cjihrig) #5388 * util: Improved “format()” performance 50-300% (Evan Lucas) #5360 PR-URL: #5559

Notable changes: * child_process: “send()” now accepts an options parameter (cjihrig) nodejs#5283 - Currently the only option is “keepOpen”, which keeps the underlying socket open after the message is sent. * constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts) nodejs#5463 * Fixed two regressions which originated in v5.7.0: - http: Errors inside of http client callbacks now propagate correctly (Trevor Norris) nodejs#5591 - path: Fixed normalization of absolute paths (Evan Lucas) nodejs#5589 * repl: “start()” no longer requires an options parameter (cjihrig) nodejs#5388 * util: Improved “format()” performance 50-300% (Evan Lucas) nodejs#5360 PR-URL: nodejs#5559

MylesBorins · 2016-03-10T21:47:37Z

adding don't land as we are not backporting the other change.

edit: the other change is under lts-watch, and now so is this

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>

trevnorris added http Issues or PRs related to the http subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 7, 2016

trevnorris force-pushed the retro-trycatch-mc branch from 1285e18 to 9653cde Compare March 7, 2016 21:19

trevnorris mentioned this pull request Mar 7, 2016

v5.8.0 proposal #5559

Merged

trevnorris force-pushed the retro-trycatch-mc branch 3 times, most recently from b7025ca to e6eac8a Compare March 7, 2016 21:40

trevnorris force-pushed the retro-trycatch-mc branch from e6eac8a to 06ec4ea Compare March 7, 2016 21:40

misterdjules mentioned this pull request Mar 7, 2016

src,http: fix uncaughtException miss in http #5571

Closed

4 tasks

misterdjules reviewed Mar 7, 2016
View reviewed changes

Fishrock123 added this to the 5.7.2 milestone Mar 7, 2016

trevnorris force-pushed the retro-trycatch-mc branch from 06ec4ea to b856c13 Compare March 7, 2016 23:37

trevnorris force-pushed the retro-trycatch-mc branch from b856c13 to d5c1024 Compare March 8, 2016 00:17

trevnorris force-pushed the retro-trycatch-mc branch from d5c1024 to 3521b05 Compare March 8, 2016 17:46

trevnorris closed this Mar 8, 2016

trevnorris merged commit 3521b05 into nodejs:master Mar 8, 2016

trevnorris deleted the retro-trycatch-mc branch March 8, 2016 17:47

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Mar 10, 2016

MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Mar 10, 2016

MylesBorins mentioned this pull request Mar 10, 2016

Track backporting AsyncWrap changes nodejs/Release#86

Closed

This was referenced May 29, 2016

Backporting Async Wrap changes #7048

Closed

[async_hooks] tracking issue nodejs/diagnostics#29

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 14, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

src,http: fix uncaughtException miss in http #5591

src,http: fix uncaughtException miss in http #5591

Uh oh!

trevnorris commented Mar 7, 2016

Uh oh!

MylesBorins commented Mar 7, 2016

Uh oh!

misterdjules Mar 7, 2016

Uh oh!

trevnorris Mar 7, 2016

Uh oh!

trevnorris Mar 7, 2016

Uh oh!

misterdjules commented Mar 7, 2016

Uh oh!

trevnorris commented Mar 7, 2016

Uh oh!

misterdjules commented Mar 7, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

misterdjules commented Mar 8, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

Uh oh!

Uh oh!

src,http: fix uncaughtException miss in http #5591

src,http: fix uncaughtException miss in http #5591

Uh oh!

Conversation

trevnorris commented Mar 7, 2016

Uh oh!

MylesBorins commented Mar 7, 2016

Uh oh!

misterdjules Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

misterdjules commented Mar 7, 2016

Uh oh!

trevnorris commented Mar 7, 2016

Uh oh!

misterdjules commented Mar 7, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

misterdjules commented Mar 8, 2016

Uh oh!

trevnorris commented Mar 8, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

Uh oh!