Suggestion: Implicitly-named error handler

[jordantogether]

Apologies if this has been already requested, if it has I've been unable to find it in the issues or elsewhere on this project page. If this is completely contradictory to the intended design of express and I'm an utter moron, then for that, I also profusely apologise

As per the documentation (https://expressjs.com/en/guide/error-handling.html), it's recommended or suggested practice to place error handlers after middleware to pick up any errors that get passed on.

However, when I am working on projects containing a lot of middleware, it is a little bit of an eyesore to keep pasting in error handlers after middleware, and also some middleware doesn't properly implement errors or doesn't catch some errors, it becomes a pain (and yes, I am raising those instances with the middleware I encounter, too).

This means that an error handler at the bottom of the middleware calls, which could be called, for want of a better phrase, a global error catcher, doesn't always work. For example, when using body-parser among other middlewares, and receiving invalid JSON, an error handler needs to be put specifically after body-parser to catch the invalid JSON.

Now, is it not possible for Express to pass back any errors it encounters iterating over middleware, back to one single function/class/whatever that the developer supplies (as a promise or whatever is desired) and cut off the chain of passing down middleware when it does that? That way, you can have all your error handling implemented in one place, and not have to copy-paste your error handler after every piece of middleware to make sure it catches the error you're looking for?

It'd still be an option to use the iterative approach, of course, but is this not a possibility in future versions?

Cheers,

[ghost]

You can put the error handlers at the bottom of your application that way the error handlers would catch all unhandled error. And you can check the type of error and decide whether to handle the error or pass it to the next error handler by calling next(err)

[jordantogether]

Hi hello-w,

I addressed this in my initial comment in this section -- this approach does not always work consistently:
This means that an error handler at the bottom of the middleware calls, which could be called, for want of a better phrase, a global error catcher, doesn't always work

And also made an explanation of why this does not always work in the introduction and latter paragraphs of my post.

[dougwilson]

Hi @jordancraw the Express.js project is made of many people and cultures, not all of which have a comprehensive graph of English, so just pointing back to your original post is not always helpful, as it may have not been understood as written.

Even myself, I read through it, but am struggling to really understand conceptually what you're asking for, as it sounds to me you are asking for something Express.js already provides. Perhaps it may help if you can provide some examples of what you are talking about in code form, which is sometimes more universal than English. For example, maybe you can provide the following:

1. Example code that shows what you think is broken.
2. Example code showing what the same code from number 1 would look like if your idea was implemented.

[ghost]

@jordancraw I guess code like router.get('path', [normalHandler, errorHandler]) is what you want. where normalHandler takes at most 3 arguments e.g. req, res, next in order and errorHandler takes exact 4 arguments err, req, res, next in order. That way, if there is an error in normalHandler you just call next(err) in normalHandler and errorHandler would be called right away. If everything is fine in normalHandler, errorHandler won't be called.

[jordantogether]

Hi @dougwilson, I appreciate that, and I didn't mean to offend anyone, but as a speaker of other languages I try to read a post before responding to it, regardless of the language.

At the moment the error handling is like this:

app.use(middleware);
app.use(my_error_handler);
app.use(some_other_middleware);
app.use(my_error_handler);
var my_error_handler = (err,req,res,next) => { if(x){ next();} }

However, having to implicitly declare error handling after each call in some instances can be a bit of a pain. What I wondered if it would not be easier to also provide the option in a future version of express to have error handling taken out of the iterative declaration it currently is, and made global for the application you're defining.

Here's an example with some pseudocode of what I mean:

// Before you declare routes or paths, register it like you register the templating language, etc
app.use('error_handler',global_error_handler)

// Pseudocode representing express handling each middleware registered with 'app'
_.each(bit_of_middleware,(err,req,res,next) {
    if(err && app.error_handler != null) { do_the_callback(app.error_handler); }
}

Thank you for replying with an answer @hello-w, and I appreciate the reply. However, what I am referring to is not the fact of using an error handler, or how to use one for the routes I'm using, as in the example you provided still it requires an error handler be implicitly defined on each path. But rather, I'm requesting the option to define an error handler once which doesn't require so much code reuse.

[dougwilson]

Thanks, I guess I misunderstood the following from your post:

This means that an error handler at the bottom of the middleware calls, which could be called, for want of a better phrase, a global error catcher, doesn't always work.

Go me it read as "global error catcher doesn't always work" but was confused because you were asking for Express to provide a mechanism for global error handling, so wasn't sure which part of the post I was reading and if you wanted global error handling or not.

Express.js has a method of a single, global error handler today. Does that not work for you? How is your proposal different from the current model of adding a single, global error handler in Express.js?

[jordantogether]

@dougwilson I might be not looking in the right places, but I've not been able to find that on the documentation I've seen on the expressjs site. Are you able to give an example of what you're referring to as a single global error handler, so I can try?

[dougwilson]

You just define one error-handling middleware last, after other app.use() and routes calls.

[jordantogether]

Okay, but what I am trying to say is -- that doesn't always work if other middleware decides to throw the error back before it reaches the end of the chain to the error-handling middleware.

For example -- if I use body-parser, then send malformed JSON, and put other middleware below the body-parser call, such as morgan, or whichever, the error won't make it down to the error handler at the end of the list as you've suggested, it just gets thrown by some other middleware which seems to see the incoming error from the previous request and send it to the client (regardless of if the error handler is set at the end of the middleware chain).

// incoming JSON: {this:is_malformed_json;}
app_use(body_parser.json({}));
app.use(this_is_my_error_handler); // <-- Error reaches here.
app.use(helmet({})); // Using helmet as an example, but this does the same with other middleware for me, the error gets thrown as soon as it goes into this.
app.use(this_is_my_error_handler) //<-- Doesn't reach here.

[dougwilson]

Hi @jordancraw in your example, the firstthis_is_my_error_handler will handle the error, os of course it doesn't reach the second this_is_my_error_handler unless you forward the error inside that using next(err). Can you try the following and see if your this_is_my_error_handler is still invoked? You should only need it once, otherwise this is a really big bug, because many people, including myself, rely on the current ability to make global error handlers.

// incoming JSON: {this:is_malformed_json;}
app_use(body_parser.json({}));
// remove this --- app.use(this_is_my_error_handler); // <-- Error reaches here.
app.use(helmet({})); // Using helmet as an example, but this does the same with other middleware for me, the error gets thrown as soon as it goes into this.
app.use(this_is_my_error_handler) //<-- Does both reach here now?

[jordantogether]

Nope, it doesn't reach it if I remove the first error handler, it only reaches the first error handler because if I remove it, the next middleware throws the error.

[dougwilson]

Here is a full example of what global error handling looks like today in Express, complete with an example invalid JSON request showing that the global error handler is invoked just fine. Let me know why this pattern doesn't work for you :) !

var express = require('express')
var bodyParser = require('body-parser')
var helmet = require('helmet')

var app = express()

app.use(bodyParser.json())
app.use(helmet())

app.get('/', function (req, res) {
  res.send('Hello, friendly user!')
})

// The single, global error handler
app.use(function (err, req, res, next) {
  res.send('Hello, I am your friendly global error handler! I saw ' + err.toString())
})

app.listen(3000)

Just save that as app.js and then install the required dependencies:

$ npm i express body-parser helmet

+ body-parser@1.17.2
+ helmet@3.6.1
+ express@4.15.3
added 67 packages in 2.959s

Then start up the app with node app.js. Go ahead and call that app with invalid JSON and the gloabl error handler responds just as expected:

$ curl -H'Content-Type: application/json' -d'{this:is_malformed_json;}' http://127.0.0.1:3000/
Hello, I am your friendly global error handler! I saw SyntaxError: Unexpected token t in JSON at position 1

[jordantogether]

Hi @dougwilson ,

I've replicated and I get the same result as you, but that's not really my contention. My contention is more with middleware that doesn't do that, and which throws an error straight away. I'll try and explain a little bit.

Suppose that there is some poor middleware you're using which doesn't pass the error call over:

app.use(middleware_1);
app.use(i_am_middleware_that_will_just_throw_an_error_in_the_result_and_not_use_next);
app.use(my_errror_handler_that_gets_no_errors);

What I'm trying to get to is, i_am_middleware_that_will_just_throw_an_error_in_the_result_and_not_use_next will not throw the error. So either I go to that middleware's repo and submit a patch that updates the middleware to call next(err), or can not express just handle the error and pass it to a function instead of relying on the error handler sitting at the end, which might not work? That way express doesn't need to rely on all middleware using next(err) properly.

If it's the case that I should just submit a patch to the repo of the middleware that doesn't do this -- I will close this issue and please accept my apologies :)

[dougwilson]

Hi @jordancraw even throwing an error will still invoke the global error handling Express.js currently has. Let's take your example and implement that in my example app to see a demonstration.

Here is the updated app.js file. I added the hypothetical i_am_middleware_that_will_just_throw_an_error_in_the_result_and_not_use_next middleware that done nothing by hard throw an error. There is no next(err) being called in the middleware. Express.js wraps al middleware invocations in a try-catch, and so thrown errors (either on purpose or accident) will still flow down the error handling framework in Express.

var express = require('express')
var bodyParser = require('body-parser')
var helmet = require('helmet')

var app = express()

app.use(bodyParser.json())
app.use(helmet())

app.use(function i_am_middleware_that_will_just_throw_an_error_in_the_result_and_not_use_next (req, res, next) {
  throw new Error('oops, error thrown!')
})

app.get('/', function (req, res) {
  res.send('Hello, friendly user!')
})

// The single, global error handler
app.use(function (err, req, res, next) {
  res.send('Hello, I am your friendly global error handler! I saw ' + err.toString())
})

app.listen(3000)

Just save that as app.js and then install the required dependencies:

$ npm i express body-parser helmet

+ body-parser@1.17.2
+ helmet@3.6.1
+ express@4.15.3
added 67 packages in 2.959s

Then start up the app with node app.js. Go ahead and call that app and the gloabl error handler responds just as expected:

$ curl http://127.0.0.1:3000/
Hello, I am your friendly global error handler! I saw Error: oops, error thrown!

I'm not saying you're not experiencing some kind of problem, but I would love to see the behavior you describe. Perhaps you can follow the format I'm using here and provide a working example that exactly demonstrates the issue you are describing. I would love to see any gaps closed in Express.js and it feels like you are holding the issue at ransom by not really helping out :) Just create an example app.js like I've been doing that has the issue you are describing, provide any setup instructions and the request to make against the server to trigger the issue.

[dougwilson]

Also, I know there are situations where people cannot provide code, for example being proprietary. You're absolutely welcome to make a pull request implementing a solution to your issue as well, of course!

I know you provided puesdo code above, but the issue is that Express.js is already hooking into all errors it can and dorwarding them to the error chain. If I were to implement it today, it wouldn't catch any errors the already-existing error handling weren't catching. That's the gap I'm trying to figure out: what is it that that middleware is doing so we can provide some mechanism for you to handle the error (like implementing your global error handling suggestion).

[jordantogether]

Yeah, that's my conundrum really. I'm sorry if it feels like I'm holding the issue at ransom, but it's not so much issue as a change of approach, which is what I'm trying to get across, but it seems we're having difficulty. It's not a tangible issue like, I have a problem getting this module to work or that -- I am using express in my daily life without issue.

To summarise what I'm trying to ask:

• At the moment you put the error handler at the end of the list of app.use() declarations.
• Instead, why not declare the error handler like you declare the templating engine (i.e. pug, handlebars) and specifically specify one error handler to call if there are errors.
• Then, when express is running each bit of middleware, when it sees the error variable isn't null, why doesn't it just fire the error off to the single error handler I implicitly defined instead?

An example of where the error-handler-at-the-end approach won't work properly is if I use someone else's middleware, and in some part of their routines, they don't call next and instead call res.send() and prevent the middleware at the end from getting the error.

var friendly_middleware = (err,req,res,next) => { if(err) { next(err); } }
var beelzebub_middleware = (err,req,res,next) => { if(err) { res.status(400).send(err);} };
var disappointed_error_handler = (err,req,res,next) => { res.status(400).send("I shouldn't even happen!"); }

app.use(friendly_middleware); // I use next()!
app.use(beelzebub_middleware); // I don't use next()
app.use(disappointed_error_handler); // I don't get this error

As a solution to this, what I envisage is instead using this kind of syntax instead:

const my_other_error_handler = require("./other_specific_error_handler");
var global_error_handler = (err) => { 
  if(err instanceof MySpecificError)
    my_other_error_handler(err);
}
app.use("global_error_handler",global_error_handler);

Then express would function in this way to process the error as it runs each middleware defined using app.use()

// Pseudocode for express looping through middleware registered to app (i.e. app.use(x), app.use(y) = [x,y])
var http_request = http.http_request_from_http_server;
var global_error_handler = app.global_error_handler_defined_above;
var list_of_middleware = app.middleware_list;
var dont_bother_continuing = false;
_.each(list_of_middleware,(individual_middleware) {
    if(!dont_bother_continuing) {
       var middleware_output = individual_middleware(http_request);
       if(middleware_output.error != null) {`
           global_error_handler(middleware_output.error);
           dont_bother_continuing = true;
       }
     } 
});

So then, in the example above:

1. friendly_middleware passes an error with next(err)
2. Express sees err was either set or error was thrown.
3. Express looks at the global_error_handler function defined with my hypothetical app.use("error_handler",global_error_handler); and passes the err variable to it.
4. Express stops processing the rest of the middleware like it does as normal.

I hope I have made some sense. If not, I'll work on submitting a patch to show what I mean ❤️

[dougwilson]

Yea, gotcha. I'm not sure how to really go about implementing such a feature. If you would like to see it implemented, please make a pull request :)

[dougwilson]

The biggest thing I'm stuck on is statements like

An example of where the error-handler-at-the-end approach won't work properly is if I use someone else's middleware, and in some part of their routines, they don't call next and instead call res.send() and prevent the middleware at the end from getting the error.

Whixh is really confusing to me, because middleware are never invoked once an error occurs, so other middleware have no way to actually stop the propogation of an error.

Maybe the trip up is just on terminology. In Express.js there are three main types of things:

1. A middleware. These are like body-parser and helmet. They have no ability to stop error propogation since they are not invoked when an error occurs. Their function signature is (req, res, next).
2. A handler. These also have no ability to stop error propogation, since they are not invoked when an error occurs. These are the function you write with the logix the responds to the request, for example by calling res.json, res.send, res.write, res.end, etc.
3. And finally error handlers. These are typically also funxtions you write, though there are a very very tiny number of them on npm, for example the errorhandler module. These will write out a response to the request detailing the error. Since they form their own stack, you can compose them such that your JSON API routes can send back a JSON API formatted error and your HTML pages can send back an HTML error, keeping that logic separate, especially useful when you decompose an app using routers.

Now, I do see in your most recent ezample of your issue that you are showing error handlers stacked on each other, not middleware. This may be why I was so confuses before. Of course if an error handler doesn't call next, that is the signal that it's responded to the request. But at the same time, these are not published to npm, so the only error handlers that wpuld normally exist in your app would be entirely your own code, so I'm confused why you can't just change them.

[jordantogether]

@dougwilson Ah okay, thanks for the clarification - my apologies if I muddied the waters. Yep, I understand what you mean -- and I am not referring to any custom error handlers from NPM, I just wrote that as a hasty example as I'm in the middle of the workday at the moment 😃

If a middleware does res.send() on the error does that not stop the propogation of the error to the error handler?

[dougwilson]

If a middleware does res.send() on the error does that not stop the propogation of the error to the error handler

It does, but it would have also stopped the propogation to even a global error handler set at the top, because Express.js would have no way to know an error ever occurred if it was not thrown or not passed to next(). How would Express.js know an error occurred at all in that case?

For example, here is how I'm interpreting your question:

app.use(function (req, res, next) {
  // this is a middleware
  // it does things like looks up a user
  //which takes 500ms from a db call
  setTimeout(function () {
    // well, the user wasn't found, but for some reason
    // this middleware is going to res.send() instead of proving
    // the error
    res.send('there is no such user :O')
  }, 500)
})

The fact that res.send() in the middleware was called isn't want stopped the chain; it was just that nothing called next() indicating that the chain should not continue. Since middleware, handlers, and error handlers can do async tasks, the fact that the middleware's main body ran to completion doesn't indicate that it has completed, thus the need for the next() function to exist.

Let me know if I'm not understanding you correctly. I am using the "middleware" in your question has I defined it in #3367 (comment) just in case we are continuing to be confused on terminology, so this way we have some grounded terms to use with specific meaning behind them :) Alternatively you can use code, of course, like I did here to demonstrate how I interpreted your question.

[dougwilson]

Oh, and just a quick note since I see you added a heart to my offer to make a pull request, if you end up needing to modify anything under the lib/router/ (https://github.com/expressjs/express/tree/master/lib/router) directory, please don't make this pull request to this repository; the contents of that are cherry-picked from the dependency backing those files https://github.com/pillarjs/router . If you make the changes there, they will get cherry picked back into this repository with the author data intact. This cherry picking is only an issue on the 4.x branch; the Express 5 branch doesn't have any of those files and instead directly requires the router module, so making the changes against the router module will also ensure that your changes are not dropped from Express 5.

[jordantogether]

Yep, you're interpreting what I said correctly. That is what I mean, and to use your terminology, an error handler would not receive the error, because the middleware (body-parser,etc) or defined handler didn't call next() for it to know that there was an error to catch.

However, say if it were practice to always throw errors in the regular JS way, rather than pass them to next() then it would be feasible to catch errors during express's execution of handlers (as you rightly corrected me on 👍 ) or middleware, and then in the catch logic, pass the error caught to the error handler defined in the global app.use() call, which would then do whatever it wanted with it.

Anyway, I've forked the pillarjs/router repo, I'll hack away on it instead of the expressjs/express/tree/master/lib/router dir. 👍