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: properly handle non-errors thrown in domains #1757

Merged
merged 3 commits into from
Mar 16, 2019

Conversation

DonutEspresso
Copy link
Member

Pre-Submission Checklist

  • Opened an issue discussing these changes before opening the PR
  • Ran the linter and tests via make prepush
  • Included comprehensive and convincing tests for changes

Issues

Restify doesn't properly handle non-errors being thrown inside the domain. The two tests augmented in this PR are currently passing unintentionally. When you dump the response sent back from restify, they actually show restify blowing up while attempting to handle the domain, and not the original non-error value thrown in the domain: TypeError: Cannot read property \'statusCode\' of undefined and AssertionError [ERR_ASSERTION]: first argument to VError, SError, or WError constructor must be a string, object, or Error (string) is required'.

I think this behavior is unintentional. @misterdjules I'm not sure if this is related to your previous domain findings, but it appears the domain is able to re-catch the secondary exception thrown by restify code attempting to handle the original user land exception.

Changes

This PR fixes the restify code and does best effort at trying to proxy the non-error values thrown by the domain back to the client.

t.ok(err);
t.equal(res.statusCode, 500);
t.equal(data.message, '');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could technically send 'undefined' over the wire, but don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well if you do throw undefined, you have a different problem as well

lib/server.js Outdated
// Expose only knonw errors
if (_.isNumber(err.statusCode)) {
// only automatically send errors that are known (e.g., restify-errors)
if (err && _.isNumber(err.statusCode)) {
Copy link
Member

@retrohacker retrohacker Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be followed for throw res;, don't know if we want to guard against that.

@rajatkumar rajatkumar self-requested a review March 14, 2019 00:57
lib/server.js Outdated
if (!(finalErr instanceof Error)) {
// if user land domain threw a string or number or any other
// non-error object, handle it here as best we can.
finalErr = (finalErr && finalErr.toString()) || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this behave if finalErr is false or 0? Do we expect both of those to return ''

@misterdjules
Copy link
Contributor

@DonutEspresso

I'm not sure if this is related to your previous domain findings, but it appears the domain is able to re-catch the secondary exception thrown by restify code attempting to handle the original user land exception.

I think you're correct that it's related to some of my recent findings re: domains.

What may be happening here is that the same domain is pushed more than once on the stack. This can happen with domains attached to HTTP requests because generally the domain associated to that requests is also associated to e.g. the socket used for that request. Since every time an event is emitted on those objects their associated domain is entered, the domain is entered when any event is emitted on the socket and subsequently when any event is emitted on the request.

FWIW, I submitted a PR to node to fix that behavior at nodejs/node#26211. I doubt it'll be merged anytime soon.

@DonutEspresso
Copy link
Member Author

@retrohacker thanks for the feedback, I redid this with some more robust logic that now reflects the thrown values.

@@ -1303,19 +1303,24 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse(
return;
}

self._emitErrorEvents(req, res, null, err, function emitError(emitErr) {
self._emitErrorEvents(req, res, null, err, function emitError() {
Copy link
Member

@retrohacker retrohacker Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to emitErr?

Copy link
Member Author

@DonutEspresso DonutEspresso Mar 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at _emitErrorEvents, and emitErr is actually the same object as err, just passed back. It seemed confusing as heck, so I removed it outright. In retrospect, I should probably remove calling this callback w/ err to begin with.

@DonutEspresso DonutEspresso merged commit cb2e717 into master Mar 16, 2019
@DonutEspresso DonutEspresso deleted the cast-domain-err branch March 16, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants