Nodebestpractices: Avoid potential memory leaks in the examples

[mcollina]

In the error handling example (https://github.com/i0natan/nodebestpractices/blob/master/sections/errorhandling/centralizedhandling.md):

// Error handling middleware, we delegate the handling to the centralized error handler
app.use(async (err, req, res, next) => {
    const isOperationalError = await errorHandler.handleError(err);
    if (!isOperationalError)
        next(err);
});

However this will result in a timeout if handleError throws.

Adding an async function as a middleware is a anti-pattern, it should be wrapped all the time and a .catch(next)  handler added (no, you cannot use a try-catch because the async function will return a promise anyway).

(If somebody is committed to using async/await, I would recommend using Hapi or Fastify).

See https://github.com/mcollina/make-promises-safe for more details.

[goldbergyoni]

@mcollina Welcome, great to have you here!

This specific example was changed to 'async' recently by a kind contributor, we didn't give the right attention to this implication.

I find it useful if we stick to the general case - wrapping Express routes with async (putting aside the async error handler middleware).

What might be wrong with the code below: do you suggest that some type of error originated in 'routeHandler' won't be caught by the wrapper? Disclaimer: simplistic code for the purpose of isolating the wrapper

const asyncMiddleware = routeHandelr =>
    (req, res, next) => {
        Promise.resolve(routeHandelr(req, res, next))
            .catch(err => {next(err)});
    };

router.get('/', asyncMiddleware(async (req, res, next) => {
    if(Math.random() < 0.5){
        throw new Error('sync error');
    }
    else{
        res.status(200).end();
    }
}));

app.use(router);

app.use((err,req, res, next) =>{
    console.log(`If we arrived here than everything is good, despite the ${err} error`);
});

[mcollina]

The code is essentially correct. I would just remove the Promise.resolve() call, as it’s superfluous.

[stale[bot]]

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚