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
feat: send 500s for unhandled requests #1777
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1352,7 +1352,10 @@ Server.prototype._finishReqResCycle = function _finishReqResCycle( | |
var self = this; | ||
var route = req.route; // can be undefined when 404 or error | ||
|
||
if (res._finished) { | ||
// if the returned err value was a string, then we're handling the | ||
// next('foo') case where we redirect to another middleware stack. don't | ||
// do anything here because we're not done yet. | ||
if (res._finished || _.isString(err)) { | ||
return; | ||
} | ||
|
||
|
@@ -1366,6 +1369,22 @@ Server.prototype._finishReqResCycle = function _finishReqResCycle( | |
var finalErr = err || res.err; | ||
req.emit('restifyDone', route, finalErr); | ||
self.emit('after', req, res, route, finalErr); | ||
} else if ( | ||
res._handlersFinished === true && | ||
res.headersSent === false && | ||
!res.err | ||
) { | ||
// if we reached the end of the handler chain and headers haven't been | ||
// sent AND there isn't an existing res.err (e.g., req abort/close), | ||
// it's possible it's a user error and a response was never written. | ||
// send a 500. | ||
res.send( | ||
new errors.InternalServerError( | ||
'reached the end of the handler chain without ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this get sent back to the client as:
Have we thought about how much this reveals about the server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I don't feel super strongly about this, and its worth noting we already send back pretty specific error messages for various other scenarios (bad redirects, bad formatters, etc). The messages are scrubbable using the error events API but makes me think we could do a better job w/ standardized error codes for restify. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it reveals anything that shouldn't be fixed. Maybe a more generic message like "request finished without response", or "sever didn't send response" is enough. |
||
'writing a response!' | ||
) | ||
); | ||
return; | ||
} else { | ||
// Store error for when the response is flushed and we actually emit the | ||
// 'after' event. The "err" object passed to this method takes | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also do the stuff from the previous if block here? Decrement inflight requests, emitting after, etc?
It seems like we are asserting along this path that this is an error, why wouldn't we clean up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling
res.send
will actually loop back to this method again, It's probably more accurate to renamefinishReqResCycle
toattemptFinishReqResCycle
, as we call it optimistically from various points in the code base and let this method handle the state reconciliation.