'after' event does not always fire before res.send() is complete. was (Response.send should handle formatter errors)

[yunong]

From #839, we should

1. return the error from the formatter if it is an instance of http or rest error.
2. return internalerror if the error is not one of those 2 instances.

[yunong]

@micahr @DonutEspresso Though about this some more, I think formatter errors should always be 500s, regardless of what kind of error they throw.

This is because firstly, we could be trying to format another error, and so at that point which error takes precedence? The original error or the one from the formatter? Secondly, if the formatter blows up, then it can only be an js InternalError seeing as it's something unexpected.

[yunong]

Made some progress in branch https://github.com/restify/node-restify/tree/845 However, there's a problem since the restify 'after' event fires independently of res.send(). see: https://github.com/restify/node-restify/blob/845/lib/server.js#L712-L714

So in the case where we res.send(200) but then the formatter blows up and we send back a 500, the after event and critically the audit logger will still continue to show a 200 in the logs.

This is not ideal and will involve a more elaborate refactor of the server code base. We could probably have res.send() emit the after event. The problem is since you can call res.send in userland handlers, we don't really want to have to make users pass in req, res, route, and error with send just so res.send() can emit an after event with those parameters.

The response object will need to be seeded with that state when it's first created for a request. The formatter error piece is done, what's now left is to rework the after event to be emitted after res.end() in res.send(), since technically that's when the response is actually over.

[DonutEspresso]

Hm, I'm not sure that formatting an error is any different from formatting regular text? If we're talking about the scenario that a user chooses to call res.send(someErr), the error is just another source of content, not an actual error from either the server or the formatter's perspective.

IFF formatting fails (regardless of the content), then I think we should return a FormatterError, we'd have to create a new type. I'd prefer not overloading InternalError for things that we have control over.

[yunong]

The problem is that there could be an error that we are already formatting,
so which one takes preference? Also, FormatterError does not help the end
user. They're not concerned with the internal implementation of the server.
As far as a consumer is concerned, if Restify fails on formatting, it is an
internalerror, and so should be treated as such. Formatters should never
fail, if it does, it's indicative of a bug of some sort, which again
relegates it to an Internalerror.

On Thursday, June 25, 2015, Alex Liu notifications@github.com wrote:

Hm, I'm not sure that formatting an error is any different from formatting
regular text? If we're talking about the scenario that a user chooses to
call res.send(someErr), the error is just another source of content, not
an actual error from either the server or the formatter's perspective.

IFF formatting fails (regardless of the content), then I think we should
return a FormatterError, we'd have to create a new type. I'd prefer not
overloading InternalError for things that we have control over.

—
Reply to this email directly or view it on GitHub
#845 (comment)
.

Via mobile

[micahr]

What is shown to the end user who is trying to use the site should be an Internal Error, while the error that is logged for debugging purposes should be the FormatterError.

[yunong]

@micahr As implemented in this branch, that's the behavior now.

[DonutEspresso]

Can we use InternalServerError instead of InternalError then? My understanding was InternalError is reserved for the uncaught exception scenarios.