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
Conversation
af676d1
to
016342a
Compare
016342a
to
0e64c09
Compare
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.
I could see how folks would feel this is a breaking change.
At the same time, also could see how not closing a response is undefined behavior and we are filling in that definition now.
Not super opinionated either way, just curious if we've considered making this a breaking change.
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this get sent back to the client as:
{ "code": "Internal Server Error", "message": "reached the end of the handler chain without writing a response!" }
Have we thought about how much this reveals about the server?
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.
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 comment
The 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.
// 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( |
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 rename finishReqResCycle
to attemptFinishReqResCycle
, as we call it optimistically from various points in the code base and let this method handle the state reconciliation.
The current behavior is no response; I'd be surprised if folks built their systems around a server that didn't respond but I guess anything is possible! This is one of those scenarios where I'm actually ok with this not being breaking, but would be curious to get other's thoughts. |
cc @cprussin |
Sweet! Thanks for the heads 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.
@DonutEspresso can we merge this for 9.x? Although this is more a fix
than a feat
IMO. No one would think that the server hanging is actually desirable.
@ghermeto merge away! |
Pre-Submission Checklist
make prepush
Issues
Closes:
Changes
If we reach the end of a handler chain and the headers haven't been sent yet, send a 500 back to the client.