defaut error handler (finalhandler) should not prevent the response being sent

[sebastien-mignot]

Or else there should be an option to provide a custom made final handler.

Why:
I use 2 error handlers in my app : one that logs the error in our database, and one that returns a 500 to the client, with an particular error message.
If for any reason I have two bits of async code that both rise an error, the app just throws the 2nd error to stderr and don't return anything (not even the first 500) to the client.

So the bug is : first error calls next(err) two times and traverses my 2 error handlers. But when next(err2) is called on the 2nd error, we are already beind my 2 error handlers in the route stack, and the finalHandler is called.

This happens often in a rest api style app (which is our case) : some route ask to do some operation on multiple business objects, we do all operations in, for instance, a "async.parallel", but due to a legitimate problems, some of those operations return an error.

In such case, I would like to configure my app to do :
log all the errors to the database
send to the client a proper error 500 with a custom message corresponding to the first error that occured (there is a good chance the next errors will have the same cause and message, but even if not, this is still the desired behaviour)

I read an argument saying that if we are sending a "normal" response to the client, and an error occur, it's preferable to stop everything, that to finish sending the response to the client and making it belive that everything is OK.
I would argue that :

1. It's usually bad design if there are still async operations being done and we are already responding to the client
2. even if we don't agree with 1, this argument doesn't apply if we are already sending an error to the client

ps : in addition to crashing and not handling the error gracefuly, in my particular application this causes even more errors, because when the client recieves a 500, it knows that there was a problème with the request. But when the server doesn't answer, the client assumes that the request was lost on the network, and it retries it 2 times at 5 sec intervals. So 2 more crashes...

[sebastien-mignot]

No interest at all for this problem ? I thought I had described it pretty thouroughly, even describing which part of the code has the problem...

This is really a big problem guys, in a production environement, it's unthinkable to not catch errors properly when they are in an asynchronous call (which is kind the point of Node)

[dougwilson]

Hi! I haven't seen the issue before and to me sounds like a code issue in your app, but maybe not. Can you provide code that reproduces the issue I can run so I know if I make a change it addresses your concern? Otherwise if you would like to make a PR, please do :)

[dougwilson]

Or else there should be an option to provide a custom made final handler.

There is. The finalhandler is only used when you don't provide the callback.

[sebastien-mignot]

var app = require('express')();

app.get('/', function (req, res, next) {
    setTimeout(function () { //do one DB query
        next(new Error("Error1"));
    });
    setTimeout(function () { //do the other (both are done in a async.parallel or something, and we respond to the client in the callback)
        next(new Error("Error2"));
    });
});

app.use(function (err, req, res, next) {
    console.log("it's ok, I caught the error: " + err); //we log the error in our db
    if (! res.headersSent) { //then we send a nicely formated response to the client, if it's the first error
        res.status(202);
        res.json({ error: "foo" });
    }
});

app.listen(3000, function () {});

[sebastien-mignot]

@dougwilson here is the code that reproduces the issue. I have no way with express to properly handle "Error2", and this case can easily happen.

In my production code, if used 2 error handler middleware, one that does the logging, and one that does if(!headersSent)... answer to the client.
I would like to be able to tell express : "if a 2nd error appends asynchronously, it must not crash the app or be given to the default finalhandler, it should be given to my logging error handler".

[sebastien-mignot]

@dougwilson to be more precise : the current problem with this test case, is that even if I have provided an error handler to log the errors, you can see that Error2 isn't passed to my error handler, and is just thrown by express and printed to stderr by node.

[dougwilson]

Yes, this is because in Express 4.x and older, it's an issue with how the router works with errors and as such, you're just not allowed to keep calling next(err) a bunch of times, as Express assumes that the 2nd+ time you call is an extremely critical error and everything is falling apart. Hopefully we can address this in Express 5.x if you can provide a PR :)

For now, the solution will be to provide your own callback to Express. Your code above would become the following:

var app = require('express')();
var http = require('http');

app.get('/', function (req, res, next) {
    setTimeout(function () { //do one DB query
        next(new Error("Error1"));
    });
    setTimeout(function () { //do the other (both are done in a async.parallel or something, and we respond to the client in the callback)
        next(new Error("Error2"));
    });
});

http.createServer(function (req, res) {
  app(req, res, function (err) {
    // implement your own finalhandler here, which is called for all errors
  })
})
.listen(3000);

[sebastien-mignot]

Thank's a lot @dougwilson , I did exaclty as in your exemple, and it works perfectly !

For the pull request, I'm not confident about doing it... But without going to the lengths of fixing how the router works, maybe it would be really easy to add a function "app.useFinalHandler(function (err, req, res) {})" to express, that just does exactly the same thing that your exemple does ?

[sjanuary]

I'd like to have a go at doing a PR for this issue if @sebastien-mignot you're not planning to? If I understand correctly, any number of calls to next(err) from some middleware should be handled by the application's own error handler if there is one, and the default final error handler should only be called if there isn't an error handler defined, or if the defined error handler calls it directly (or if the defined error handler throws an error itself?).

[sebastien-mignot]

@sjanuary Hi. I'm not planning to, it would be nice indeed if you did !
And your description of the desired solution seems perfect to me.

[sjanuary]

I've written a (currently failing) test for this, based on Sebastien's example code. I'd appreciate any feedback, I've just posted the code here for now:

    it('should call a final error handler twice when two errors are thrown', function(done){
        // Should complete in less than 500ms.  This value is arbitrary but should be long enough to allow both errors to be thrown.
        this.timeout(500);
        var app = express();
        var caughterror1 = false;
        var caughterror2 = false;
        app.get('/', function (req, res, next) {
            setImmediate(function () { // make one async call
                next(new Error("Error1"));
            });
            setImmediate(function () { // make a second async call
                next(new Error("Error2"));
            });
        });

        app.use(function (err, req, res, next) {
            if(err.message === "Error1") {
                caughterror1 = true;
            }
            if(err.message === "Error2") {
               caughterror2 = true;
            }
            if (!res.headersSent) { // Send a 202 response to the client if it's the first error
                res.status(202);
                res.json({ error: "Error Received" });
            }
            if(caughterror1 && caughterror2) {
                done();
            }
        });

        request(app)
        .get('/')
        .expect(202)
        .end();
    })

[sebastien-mignot]

@sjanuary "app.use(function (err, req, res, next) {" is exactly what we write to add a standard error handler (one that follows the "next" middleware chain, and can't be called anymore one it's position in the middleware chain is passed).
You need to use another syntax to differentiate this with a final error handler that will always be called, e.g. :
app.useFinalHandler(function (err, req, res, next) {

[wesleytodd]

@sebastien-mignot I think that adding new api surface area for this issue is not the best way to go. It almost never is on a library like express, causes even more support issues.

@sjanuary I think a solution more similar to doug's original example would be better for this. Which I don't think requires any new code in express itself to handle. Also, I think you should have a look at making a change like this over here as per Doug's suggestion:

Hopefully we can address this in Express 5.x if you can provide a PR

I don't think closing this issue is necessary as it is a nice way to track any changes, but the new router is probably a better place to make any changes :)

[sebastien-mignot]

@wesleytodd It's not really "a new api different that doug's example", what I suggest is more : "packaging doug's example so it's not 100% impossible to understand and guess for a standard express user".

[wesleytodd]

Strictly speaking a new method is a new api. After a little more thought, it should be fairly simple to use the existing api to do this. For example:

app.set('final handler', function (err, req, res, next) {
  // my custom handler
});

Then in here you just need to do something like this:

var done = callback || this.get('final handler') || finalhandler(req, res, {
  env: this.get('env'),
  onerror: logerror.bind(this)
});

This way there is no new api. It still requires the change above, but not much else. Thoughts?

[sebastien-mignot]

Yes, app.set("final handler", function...) is as good for me as app.useFinalHandler(function...)
The method could just be a 1 line function that does the app.set thing, I'm a bit confused as to how that complexifies the api so much ; but anyway that's for express coders to decide, both solutions are fine for us, as long as set("final handler".. is clearly explained in the documentation (for me it's simpler to maintain a doc about methods, instead of having app.set that has a huge list of possible arguments, but whatever)

[sjanuary]

When I wrote the test I was envisaging changing the default behavior rather than adding a new API, so that calling next(err) multiple times would call the user's final error handler multiple times instead of calling the default one after the first time.

Personally I would consider even the app.set option an API change since it would have to be documented in the settings table and therefore supported.

Additionally, app.set seems to be currently used for changing settings e.g. boolean values and I don't think using it to add functionality is a good fit. It could possibly be used to control which final handler is used for multiple errors though:
e.g. app.set("own final handler", true); would enable your own final handler to be used multiple times, it would default to false, which is the current behavior. "own final handler" probably isn't the best name for it, I think that needs some further thought.

@wesleytodd I agree this should be targeted at the 5.x stream rather than current release due to the potential to break applications that rely on existing behavior.

[sjanuary]

Would someone with the right permissions be able to tag this issue with 5.0 or 5.x? @ritch ?

[bnoordhuis]

@sjanuary Done.

[sjanuary]

Thank you @bnoordhuis

[wesleytodd]

Lol, yeah @sjanuary I did back peddle on the api changes. A settings addition is also an api change. For what it is worth, app.set is actually used for similar behavior already. See the query parser, etag and trust proxy settings: https://github.com/strongloop/express/blob/master/lib/application.js#L363-L380

Aside from that, I am not a huge fan of this change in behavior. The nice linear, single run behavior of middleware is simple to understand and good as is. This feature request is all built around the use case of logging errors in an error middleware, which you should not be doing anyway. Logging should occur at the location of the error condition in your application, otherwise you lose the context of your stack trace, making debugging much more difficult. Promoting error logging in this fashion is not something I would recommend. If there is another use case that is more compelling then maybe you could get me behind it, but as it stands I don't see changing the current middleware behavior as worth if for this situation.

EDIT: disregard tangential comments :)

[sebastien-mignot]

@wesleytodd What you say is really not serious... You can't have code that acces the database storing the error (or the file or whatever) that is replicated throughout all the code...
There should absolutely be an error handler that takes care of logging the error. And btw the context is not lost at all, that's the point of giving the error as argument to next !!!

[wesleytodd]

I am sure there are many ways to do error logging that work. We have tried using the default handler to log errors in our production apps and have found it is much better to log where the error occurs. That was tangential to the issue at hand, sorry I mentioned it.

[dougwilson]

Since this issue, there is a proposed fix for this in the router module's repository: throw an error if you call next() more than once. next() is a callback and you're only allowed to call it once. Calling it twice is a critical error, and of course finalhandler cuts off the request, because as far as it can tell, it was told an error occurred, but a response in in process. As far as it knows, the response may now never complete, so it terminates the connection.

I don't particularly like any of the proposals to the API in this issue so far, and there is no reason why you cannot just use the example in #2793 (comment) to do your logging. This is not a secret API and has existed for a really long time. If the argument is that it's not documented, then that would just be a failing of our documentation that I'm sure can easily be corrected.

[sjanuary]

Thanks for your input @dougwilson. So it seems like since Router has gone the other way to tighten this up so that calling next() more than once is illegal then adding any kind of support for multiple next() calls would not be desirable. As you said, it would be good to take a look at the documentation and make sure it covers this.

[sebastien-mignot]

@dougwilson The example works indeed well, and it not at all documented, so it would need to be if that's the kept solution. (and I still think the syntax isn't "intuitive", but ok)

Your first comment makes me anxious though : you can't call "next" multiple times ? So when an error occurs (a true error, not one that is nicely caught and returned in e.g. callback(err, res)), what are we supposed to do ? It's already tedious to write wrappers that catch the errors and pass them to next, if we must implement a global system that catch errors, retain them while the other asynchronous stuff execute, then make a sum-up at the end of the route asynchronous computation and call next only once, it's gonna be more clutter than just handling every error ourselves and never use express for this.

[sjanuary]

For reference, pillarjs/router#26 is the PR for not allowing next() to be called multiple times.

[sjanuary]

Closing this for now as I think any further discussion would be better to have in the router project.