New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,http: fix uncaughtException miss in http #5591

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
4 participants
@trevnorris
Contributor

trevnorris commented Mar 7, 2016

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/

@trevnorris trevnorris referenced this pull request Mar 7, 2016

Merged

v5.8.0 proposal #5559

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Mar 7, 2016

Member

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

Member

MylesBorins commented Mar 7, 2016

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

@misterdjules misterdjules referenced this pull request Mar 7, 2016

Closed

src,http: fix uncaughtException miss in http #5571

3 of 4 tasks complete
@@ -1198,7 +1198,10 @@ Local<Value> MakeCallback(Environment* env,
}
if (ret.IsEmpty()) {
return Undefined(env->isolate());
if (callback_scope.in_makecallback())

This comment has been minimized.

@misterdjules

misterdjules Mar 7, 2016

Contributor

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

@misterdjules

misterdjules Mar 7, 2016

Contributor

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

This comment has been minimized.

@trevnorris

trevnorris Mar 7, 2016

Contributor

Can do.

@trevnorris

trevnorris Mar 7, 2016

Contributor

Can do.

This comment has been minimized.

@trevnorris

trevnorris Mar 7, 2016

Contributor

done, just below.

@trevnorris

trevnorris Mar 7, 2016

Contributor

done, just below.

@Fishrock123 Fishrock123 added this to the 5.7.2 milestone Mar 7, 2016

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Mar 7, 2016

Contributor

@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?

Contributor

misterdjules commented Mar 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Mar 7, 2016

Contributor

@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.

Contributor

trevnorris commented Mar 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Mar 7, 2016

Contributor

@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?

Contributor

misterdjules commented Mar 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Mar 8, 2016

Contributor

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

Contributor

trevnorris commented Mar 8, 2016

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

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Mar 8, 2016

Contributor

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

Contributor

trevnorris commented Mar 8, 2016

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

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Mar 8, 2016

Contributor

@trevnorris LGTM. Thank you!

Contributor

misterdjules commented Mar 8, 2016

@trevnorris LGTM. Thank you!

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.

Fixes: #5555
PR-URL: #5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Mar 8, 2016

Contributor

Thanks much! Landed in 3521b05.

Contributor

trevnorris commented Mar 8, 2016

Thanks much! Landed in 3521b05.

@trevnorris trevnorris closed this Mar 8, 2016

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

@trevnorris trevnorris deleted the trevnorris:retro-trycatch-mc branch Mar 8, 2016

Fishrock123 added a commit that referenced this pull request Mar 8, 2016

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.

Fixes: #5555
PR-URL: #5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>

Fishrock123 added a commit that referenced this pull request Mar 8, 2016

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.

Fixes: #5555
PR-URL: #5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>

Fishrock123 added a commit that referenced this pull request Mar 8, 2016

2016-03-08, Version 5.8.0 (Stable)
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

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 9, 2016

2016-03-08, Version 5.8.0 (Stable)
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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Mar 10, 2016

Member

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

Member

MylesBorins commented Mar 10, 2016

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

MylesBorins added a commit that referenced this pull request Jun 7, 2016

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>

MylesBorins added a commit that referenced this pull request Jun 24, 2016

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>

MylesBorins added a commit that referenced this pull request Jun 28, 2016

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>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

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>

@MylesBorins MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment