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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 20 additions & 1 deletion lib/server.js
Expand Up @@ -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;
}

Expand All @@ -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(
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.

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.

'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
Expand Down
42 changes: 42 additions & 0 deletions test/server.test.js
Expand Up @@ -2497,6 +2497,48 @@ test('should emit error with multiple next calls with strictNext', function(t) {
});
});

test(
'should send 500 if we reached the end of handler chain w/o sending ' +
'headers',
function(t) {
var server = restify.createServer({
dtrace: helper.dtrace,
strictNext: true,
log: helper.getLog('server')
});
var client;
var port;

server.listen(PORT + 1, '127.0.0.1', function() {
port = server.address().port;
client = restifyClients.createJsonClient({
url: 'http://127.0.0.1:' + port,
dtrace: helper.dtrace,
retry: false
});

server.get('/noResponse', function(req, res, next) {
next();
});

client.get('/noResponse', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 500);
t.equal(err.name, 'InternalServerError');
t.equal(
err.message,
'reached the end of the handler chain without ' +
'writing a response!'
);
client.close();
server.close(function() {
t.end();
});
});
});
}
);

test('uncaughtException should not trigger named routeHandler', function(t) {
SERVER.get(
{
Expand Down