Express error handler not catching an error properly

Question

I'm working on a small express project to create and check keys. I have 2 custom errors that I throw when needed, InvalidKeyError and NotFoundError. I am using an error handling middleware:

import { Request, Response, NextFunction } from 'express';
import { CustomError } from '../errors/custom-error';

export const errorHandler = (
    err: Error,
    req: Request,
    res: Response,
    next: NextFunction
) => {
    if (err instanceof CustomError) {
        console.log(err);
        return res.status(err.statusCode).send({ errors: err.serialiseErrors() });
    }

    res.status(400).send({
        errors: [{ message: 'Something went wrong' }]
    });
};

When NotFoundError is called I get the following JSON (which is what I want):

{
    "errors": [
        {
            "message": "Page does not exist"
        }
    ]
}

But when I throw InvalidKeyError my app crashes, but it does show me the error message in the console. I want it to throw like my NotFoundError.

My InvalidKeyError and NotFoundError both extend my abstract class CustomError. I throw a NotFoundError in my index.ts file when a route that does not exist is being requested. I call my InvalidKeyError from a router that my index.ts uses.

This is index.ts:

import express from 'express';
import { json } from 'body-parser';
import mongoose from 'mongoose';

import { createAPIKeyRouter } from './routes/create-api-key';
import { checkAPIKeyRouter } from './routes/check-api-key';

import { errorHandler } from './middlewares/error-handler';
import { NotFoundError } from './errors/not-found-error';

const app = express();
app.use(json());

app.use(createAPIKeyRouter);
app.use(checkAPIKeyRouter);

app.all('*', (req, res) => {
    throw new NotFoundError();
});

app.use(errorHandler);

const start = async () => {
    try {
        await mongoose.connect("mongodb://localhost:27017/coupons");
        console.log("Connected to MongoDb");
    } catch (err) {
        console.error(err);
    }

    app.listen(4000, () => {
        console.log("Listening on port 4000");
    });
};

start();

and the code for the router that my app uses:

import express from 'express';
import { InvalidKeyError } from '../errors/invalid-api-key-error';
import { APIKey } from '../models/api-key-model';
import { Hash } from '../services/hash';

const router = express.Router();

router.get('/api/checkkey', async (req, res) => {
    const suppliedKey = req.get('key');
    const suppliedSecret = req.get('secret');

    if (!suppliedKey || !suppliedSecret)
        throw new InvalidKeyError('Key or secret not passed');

    const storedAPIKey = await APIKey.findOne({ key: suppliedKey });

    if (!storedAPIKey)
        throw new InvalidKeyError('Key does not exist');

    if (!Hash.compareKeys(suppliedKey, suppliedSecret, storedAPIKey.salt))
        throw new InvalidKeyError('Key and secret do not correspond')

    res.send('Hi there');
});

export { router as checkAPIKeyRouter };

Answer 1

You're declaring your route handler async and then throwing inside of it. That means your route handler will return a promise that gets rejected when you throw.

BUT, Express doesn't pay any attention at all to the promise that your route handler returns - in fact, Express doesn't pay any attention to any value returned by your route handler. So, when you throw and cause that returned promise to get rejected, nobody is listening for that rejection.

You will need to either use your own try/catch inside the route handler to catch your throws or you will need to wrap your route handler in a utility function that catches rejections from the promise you're returning.

Here's using your own try/catch:

router.get('/api/checkkey', async (req, res, next) => {
    try {
        const suppliedKey = req.get('key');
        const suppliedSecret = req.get('secret');

        if (!suppliedKey || !suppliedSecret)
            throw new InvalidKeyError('Key or secret not passed');

        const storedAPIKey = await APIKey.findOne({ key: suppliedKey });

        if (!storedAPIKey)
            throw new InvalidKeyError('Key does not exist');

        if (!Hash.compareKeys(suppliedKey, suppliedSecret, storedAPIKey.salt))
            throw new InvalidKeyError('Key and secret do not correspond')

        res.send('Hi there');
    } catch(err) {
        next(err);
    }
});

And, here's using your own wrapper function (which you can share with many methods like this:

function asyncHandler(fn) {
    return async function(req, res, next)  {
        try {
            await fn(req, res, next);
        } catch(err) {
            next(err);
        }
    }
}


router.get('/api/checkkey', asyncHandler(async (req, res) => {
    const suppliedKey = req.get('key');
    const suppliedSecret = req.get('secret');

    if (!suppliedKey || !suppliedSecret)
        throw new InvalidKeyError('Key or secret not passed');

    const storedAPIKey = await APIKey.findOne({ key: suppliedKey });

    if (!storedAPIKey)
        throw new InvalidKeyError('Key does not exist');

    if (!Hash.compareKeys(suppliedKey, suppliedSecret, storedAPIKey.salt))
        throw new InvalidKeyError('Key and secret do not correspond')

    res.send('Hi there');
}));

And, here's how you could add methods to Express that are promise-aware: Async/Await in Express Middleware so you can just do this:

// note this is router.getP()
router.getP('/api/checkkey', async (req, res) => {
    const suppliedKey = req.get('key');
    const suppliedSecret = req.get('secret');

    if (!suppliedKey || !suppliedSecret)
        throw new InvalidKeyError('Key or secret not passed');

    const storedAPIKey = await APIKey.findOne({ key: suppliedKey });

    if (!storedAPIKey)
        throw new InvalidKeyError('Key does not exist');

    if (!Hash.compareKeys(suppliedKey, suppliedSecret, storedAPIKey.salt))
        throw new InvalidKeyError('Key and secret do not correspond')

    res.send('Hi there');
});