Skip to content
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 #5571

Closed
wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Description

If the call to MakeCallback is currently in_makecallback() then return
the empty handle instead of Undefined. This allows the error to
propagate through multiple MakeCallback calls.

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

@mscdex mscdex 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 5, 2016
@misterdjules
Copy link

It seems 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) {

would fix the issue while not missing any async-wrap calls.

@trevnorris
Copy link
Contributor Author

@misterdjules genius. have one small alteration on that, but makes sense.

If the call to MakeCallback is currently in_makecallback() then return
the empty handle instead of Undefined. This allows the error to
propagate through multiple MakeCallback calls.
@trevnorris
Copy link
Contributor Author

@misterdjules That worked perfectly. Thanks for the idea. Reason I only return if in_makecallback() is to preserve the user-facing API.

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

@MylesBorins
Copy link
Contributor

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

Edit: CI failures are on a single arm machine and appear to all be infra related.

@trevnorris
Copy link
Contributor Author

@thealphanerd should this have landed on master first? There aren't any conflicts so I can move it over if needed.

@MylesBorins
Copy link
Contributor

I think we should close this and make a new PR against master. we should then likely aim for a release on Monday. I'm not feeling super comfortable pushing through a release without review at 9:30pm on Friday.

@rvagg @jasnell @nodejs/ctc please feel free to chime in here. For those of you just joining us v5.7.1 has a nasty regression dealing with errors in http client... this PR will fix that.

@misterdjules
Copy link

@thealphanerd

For those of you just joining us v5.7.2 has a nasty regression

Isn't 5.7.1 the release that introduced the regression and 5.7.2 the one that would fix it?

@MylesBorins
Copy link
Contributor

@misterdjules that is correct... this is exactly why I didn't want to push out the release last night :P

@jasnell
Copy link
Member

jasnell commented Mar 7, 2016

LGTM

@Fishrock123 Fishrock123 modified the milestone: 5.7.2 Mar 7, 2016
@misterdjules
Copy link

@trevnorris

Reason I only return if in_makecallback() is to preserve the user-facing API.

What do you mean by "the user-facing API"? Do you mean the API of AsyncWrap::MakeCallback that is used by node's core internally or something else?

It seems that AsyncWrap::MakeCallback may already return an empty handle in some cases when it's not a recursive call:

  1. When the next tick queue is empty.
  2. When the next tick queue is not empty and its processing does not trigger an error.

Also, it seems the only place in core where AsyncWrap::MakeCallback's return value is used is node::JSStream's implementation.

So my question is: could we just make node::MakeCallback and node::AsyncWrap::MakeCallback always return an empty value if the underlying callback call threw an error instead of undefined and just update the few internal uses that would need to check the value before using it?

@trevnorris
Copy link
Contributor Author

Will open PR on master shortly.

@trevnorris trevnorris closed this Mar 7, 2016
@trevnorris trevnorris deleted the retro-trycatch-mc branch March 7, 2016 21:13
@trevnorris
Copy link
Contributor Author

@misterdjules Just saw your comment. For 1. Will ret be forcefully set to an empty handle if a nextTick callback throws? I didn't believe it would. And if ret was an empty handle to start then it wouldn't have gotten down that far in the function. This is the same for 2.

My concern is about making node::MakeCallback return an empty handle, since it would be change of public API and these changes are meant to be backported to v4. Though on that note it does seem that changing AsyncWrap::MakeCallback() to return an empty handle wouldn't be a problem.

In all honesty this should change to use the MaybeLocal API, but that's definitely a major change that can't be backported. So I'm holding off.

@misterdjules
Copy link

@trevnorris

For 1. Will ret be forcefully set to an empty handle if a nextTick callback throws? I didn't believe it would. And if ret was an empty handle to start then it wouldn't have gotten down that far in the function. This is the same for 2.

Right, I misinterpreted that code and my analysis doesn't make sense, my apologies for the confusion :(

My concern is about making node::MakeCallback return an empty handle, since it would be change of public API and these changes are meant to be backported to v4.

Yes, I had also missed that this PR was making the same change to node::MakeCallback. That makes sense.

Though on that note it does seem that changing AsyncWrap::MakeCallback() to return an empty handle wouldn't be a problem.

I saw you made that change in #5591, thank you!

In all honesty this should change to use the MaybeLocal API, but that's definitely a major change that can't be backported. So I'm holding off.

That makes sense too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants