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

feat: send 500s for unhandled requests #1777

Merged
merged 1 commit into from Jul 2, 2020
Merged

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

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.

Copy link
Member

@retrohacker retrohacker left a 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 ' +
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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(
Copy link
Member

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?

Copy link
Member Author

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.

@DonutEspresso
Copy link
Member Author

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.

@DonutEspresso
Copy link
Member Author

cc @cprussin

@cprussin
Copy link
Contributor

cprussin commented May 7, 2019

Sweet! Thanks for the heads up

Copy link
Member

@ghermeto ghermeto left a 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.

@DonutEspresso
Copy link
Member Author

@ghermeto merge away!

@ghermeto ghermeto merged commit 885cecd into master Jul 2, 2020
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.

None yet

6 participants