Express: Default error handler is used even though last error handler in stack doesn't call `next`

[adamreisnz]

I stumbled upon this peculiar scenario in my app which I haven't been able to reproduce in an isolated case yet.

The situation is as follows:

I have a stack of error handling middleware, with the last one in the stack sending an error response to the client and ending the response. This last middleware does _not_ call next.

However, despite this, I see the error being logged in my console _after_ the error respones has been sent. So for some reason, Express decided that I haven't handled my error and is using the default error handler instead.

While debugging this, I placed _another_ empty error handler after my existing stack which only logs to console. With this error handler in place, the default error handler was no longer being called.

Thinking it was an issue with my middleware stack, I disabled all of them, and only left this test handler in my code. This caused the problem to reapper, and Express started using the default handler again:

app.use(function(err, req, res, next) {
  console.log('last handler');
});
//after this error handler, express logged the error to console with the default handler

Only after adding a _second_ empty handler did the default behaviour stop again:

app.use(function(err, req, res, next) {
  console.log('one before last handler');
});
app.use(function(err, req, res, next) {
  console.log('last handler');
});
//now express was quiet

This seems really dodgy to me. I didn't dive into Express' source code yet, but I'll go have a look if there something about my error format or something else in my app that could be triggering this.

However, I do think it might be an Express issue with the default error handler being called inappropriately under some circumstances. Will report back if I find out more.

[dougwilson]

This issue is caused by something calling next(err) more than once in your code. This is exactly what we expected to happen on that case. You can reproduce that with a middleware that just calls next(err) twice.

If you don't think this is the case, please provide us with a reproduction case or a PR and we'll take a look/re-open this issue!

[adamreisnz]

Thank you @dougwilson. I will investigate and see if this is the case.

Could you please update the documentation around the default error handler to mention this? It would have saved me a lot of searching and debugging.

[adamreisnz]

@dougwilson you were right, it was a particular passport strategy that had a fall through and was calling both the success and failure, which probably propagated into two separate next calls. 👍

[LinusU]

Is there a reason for being able to call next twice, or would it be better to throw an error if it's attempted?

[adamreisnz]

@LinusU I can't think of any reason, so maybe a warning (or error) would be reasonable. Is there a use case for calling next more than once?

Personally, I find it a rather odd side effect that the default error middleware is called when that happens, as it sent me on a long quest to try and find an issue with my error middleware stack, while in fact the issue was in a completely different part of the application.

[AndyOGo]

@dougwilson
@adamreisnz
@LinusU
I have the same problem.

My custom error handlers gets called, all error logs work, but it's res.send('My custom Error Page') is ignored. So to clarify the custom error handler is called, but it's payload is overwritten by the default error handler of express 😲

Here is some code to reproduce it:

const express = require('express');

const app = express();
const port = 3000;

app.all(function(req, res, next) {
  res.status(404);

  // neither asynchronous error forwarding works
  next(new Error('Not Found'));

  // nor synchronous throw works
 throw new Error('Not Found');
}

// put custom error message middleware last as describe in the guide
// @link: https://expressjs.com/en/guide/error-handling.html#writing-error-handlers
app.use(function costomErrorHandler(err, req, res, next) {
  // error logging works as expected
  console.error(err)

  // the client will never get this payload
  // instead it receives the default error handler's payload
  res.send('My custom error Page does not work');
})

app.listen(port, () => console.log(`Example app listening on port ${port}!`))

@dougwilson
Please reopen

I assume that the error happens somewhere:

https://github.com/expressjs/express/blob/dc538f6e810bd462c98ee7e6aae24c64d4b1da93/lib/application.js#L149-L175

And here the related code of finalhandler:

https://github.com/pillarjs/finalhandler/blob/92c4ea8892a372ac017654560e5c5484b6e9f590/index.js#L97-L116

[dougwilson]

Hi @AndyOGo sorry you're having issues. This issue is 2 years old and cannot be reopened (button is disabled by GitHub). The OP in this issue did resolve their issue, so the original issue is indeed resolved. Rather than hijacking a 2 year old issue anyway, I would suggest opening a new issue. Things can change over 2 years, including regressions. I have not had a chance to look into the issue, but with your post hiding deep in an old, closed issue, it's unlikely others will see it and help vs when I eventually get some time to take a look.

Just open a new issue with your comment above and you can link to this issue if you think it's needed.