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

fix server not propagating errors on prehandler(promise) + handler error (#3242) #3358

Merged
merged 3 commits into from Nov 29, 2016

Conversation

@turtleDev
Copy link
Contributor

@turtleDev turtleDev commented Oct 3, 2016

This patch proposes a fix for #3242 ;

The fix revolves around explicit domain bindings and execution of promise handler from the event loop (using process.nextTick) so that the error is not swallowed up by the promise itself (which was basically leading to a deadlock; the promise never fully resolved, and the callback is never called )

This is technically a bugfix, and I wasn't sure if I'm supposed to increase the patch version number, so I'll leave it up to you guys.

turtleDev added 2 commits Oct 3, 2016
This commit fixes hapijs#3242; The fix basically revolves around
running the promise handler on the nextTick, so that it would
propogate upwards, but also explicitly binding a domain to a
handler up in the chain of propogation, so that it's handled
properly.
@turtleDev turtleDev changed the title fix #3242 fix server hanging up on prehandler(promise) + handler error (#3242) Oct 3, 2016
@turtleDev turtleDev changed the title fix server hanging up on prehandler(promise) + handler error (#3242) fix server not propagating errors on prehandler(promise) + handler error (#3242) Oct 3, 2016
@johnbrett
Copy link
Contributor

@johnbrett johnbrett commented Oct 5, 2016

Can confirm this fixes #3242 - Thank you for this @turtleDev!

@turtleDev
Copy link
Contributor Author

@turtleDev turtleDev commented Oct 5, 2016

@johnbrett it was a pleasure 😄

if (source instanceof Error) {
return next(Boom.wrap(source));
}
// do not use Hoek.nextTick because it will not work as intended
Copy link
Contributor

@Marsup Marsup Oct 5, 2016

I see no reason why hoek's wouldn't work here.

Copy link
Member

@nlf nlf Oct 5, 2016

hoek's nextTick drops context so this would become invalid in this case

Copy link
Contributor

@Marsup Marsup Oct 5, 2016

With an arrow function this shouldn't matter at all.

Copy link
Member

@nlf nlf Oct 5, 2016

that's a good point, what didn't work for you here @turtleDev?

Copy link
Contributor Author

@turtleDev turtleDev Oct 5, 2016

tbh, I'm not entirely sure. I tried using Hoek.nextTick but it didn't somehow result in stack unwinding. maybe it has something to do with how domains work, but that's just a very un-informed guess.

My bad, Hoek.nextTick will work, but it's not really suited for this situation. I looked into it, and it turns out, Hoek.nextTick wraps up a function in process.nextTick and returns the wrapped function ... but that means that you have to call the wrapped function, which here would seem redundant.

Copy link
Contributor

@Marsup Marsup Oct 5, 2016

Copy link
Contributor Author

@turtleDev turtleDev Oct 6, 2016

ahh yes, I realized this after I updated my comment. Thanks for pointing that out.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Oct 5, 2016

Looks very similar to the previous PR, doesn't exit the server anymore @johnbrett ?

@johnbrett
Copy link
Contributor

@johnbrett johnbrett commented Oct 5, 2016

Yep - very similar @Marsup. It seems to be be that not using Hoek.nextTick is the only difference, I don't really understand why it makes the difference though :/

@turtleDev
Copy link
Contributor Author

@turtleDev turtleDev commented Oct 5, 2016

@johnbrett Hoek.nextTick will work too, see #3358 (comment)

@turtleDev
Copy link
Contributor Author

@turtleDev turtleDev commented Oct 28, 2016

@johnbrett @Marsup Hello guys! Any updates on when this PR will be reviewed and /or merged?

@Marsup
Copy link
Contributor

@Marsup Marsup commented Oct 28, 2016

I'm 👍 on this, we're not the ones to convince, @hueniverse is.

@hueniverse hueniverse added the bug label Nov 29, 2016
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse merged commit 10bcdaa into hapijs:master Nov 29, 2016
2 checks passed
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants