Why is my error handler in ExpressJS not working?

Question

I am building an api, with ExpressJS, for a project of mine and would like to implement error handling. I've read some articles on the internet on how to do this and tried a few ways of implementing error handling in my api. The problem that I have now is that my error handler doesn't work and I have no idea why. To make my problem more clear, here's what I already tried:

This is the bottom part of my index.js file:

// Controllers
app.use("/auth", require("./controllers/auth"));
app.use("/clients", require("./controllers/clients"));

// Error handling
app.use((err, req, res, next) => {
    logger.error(
        `date - ${new Date()}, message - ${err.message}, stack trace - ${
            err.stack
        }`
    );

    return res
        .status(500)
        .json({ status: 500, message: "Something went wrong." });
});

const PORT = process.env.PORT || 3001;
app.listen(3001, () => {
    console.log(`Listening on port ${PORT}`);
});

As a side note: I am using WinstonJS for logging errors to files and Mongoose as the ORM for my MongoDB database.

This is the controller for the clients route (controllers/clients.js):

const express = require("express");
const router = express.Router();

const ClientsService = require("../services/clients");

// GET -> Get all clients -> /clients
router.get("/", async (req, res) => {
    const clients = await ClientsService.getAll();

    return res.status(200).json({ status: 200, data: clients });
});

module.exports = router;

And finally, this is the clients service (services/clients.js), the place where I would like to throw errors (maybe this isn't the right place to throw errors, please correct me if I am wrong):

const ClientModel = require("../models/Client");

const getAll = async () => {
    return ClientModel.findById({})
        .then(clients => clients)
        .catch(err => {
            throw new Error(err);
        });
};

module.exports = { getAll };

As you can see, I put an empty object in the function for findById. This is to force an error, so that I can test the error handling. A few lines below that, I throw an error. I expect this to fire the middleware function that I defined in index.js. That middleware function, as shown in the code for index.js, logs the error to a file and then sends a 500 response to the client (read 'user').

Instead of this, I actually get an error in the console called 'UnhandledPromiseRejectionWarning'. Here's the error:

(node:15416) UnhandledPromiseRejectionWarning: Error: CastError: Cast to ObjectId failed for value "{}" at path "_id" for model "clients"
    at D:\projects\myproject\services\clients.js:7:10
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async D:\projects\myproject\controllers\clients.js:8:18
(node:15416) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:15416) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise 
rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Why is my error handling not working? If you have a solution, how would I be able to enable error handling for my code?

Thank you in advance!

Answer 1

If it helps anyone, you should be able to get this down to fairly tidy syntax like this:

this._expressApp.get('/api/somedata', this._catch(this._router.myDataRoute));

Some example code of mine here - see also the blog post from Alex Bazhenov.

Answer 2

In the express error handling documentation, it says:

You must catch errors that occur in asynchronous code invoked by route handlers or middleware and pass them to Express for processing.

So you need to catch the error, and pass it to the express error middleware using next(err):

So your controller needs to be updated like this:

router.get("/", async (req, res, next) => {
  try {
    const clients = await ClientsService.getAll();
    return res.status(200).json({ status: 200, data: clients });
  } catch (err) {
    next(err);
  }
});

Also you had better to return a promise from your service like this which is clearer:

const getAll = () => {
  return ClientModel.findById({});
};

Now if you are concerned to use try catch in every route, you may check this answer to avoid it.

Answer 3

Make the try/catch before calling the async function

try{
    const clients = await ClientsService.getAll();
    return res.status(200).json({ status: 200, data: clients });
}catch(err){
    return res.status(500).json({ status: 500, message: 'There was a problem :' + err });
}