Zone.js for async error handling

[ywarezk]

Currently when i create a new express application. That application has a default error handling. So code like this:

const express = require("express");

const app = express();

app.get("*", function() {
    throw new Error("something happened");
})

app.listen(3000, function() {
    console.log("we are now listening to port 3000");
});

Will cause the default express error handler to kick in. The js process does not crash and everything works as expected.

For async error the behaviour is different and my question is why? with Zone.js we can now do a similar behaviour. For example:

const express = require("express");

const app = express();

app.get("*", function() {
    setTimeout(() => {
        throw new Error("something happened");
    }, 500);    
})

app.listen(3000, function() {
    console.log("we are now listening to port 3000");
});

the above code will crash the process.

According to express docs. we can handle sync error but the async error you will have to call next with the error. So the above code should be like so:

app.get("*", function(req, res, next) {
    setTimeout(() => {
        next(new Error("something happened"));
    }, 500);    
})

So this behaviour seems non ideal to me, cause in my opinion, with zone.js in the picture we can reach a much better results. I would love to open a discussion on the matter... The ideal is similar behaviour with sync and async errors (which is quite possible with zone.js to identify error that originate from async code inside express middleware functions). so to summarise I would see the async error handling working like this:

• if we are using next(new Error()) then great let's keep the same behaviour.
• if not, determine with zone.js if we have exception in our async code that originated from one of our middleware functions, and pop the default error handler (and of course the process will not crash).
• This will result in much stable apps (process won't be close on async errors) and another added bonus is better debugging, and better error messages and stack traces of the exceptions in our async code.

[fed135]

This is relevant to a discussion in the web server frameworks working group: https://github.com/nodejs/web-server-frameworks/issues/22

[ywarezk]

This is relevant to a discussion in the web server frameworks working group: nodejs/web-server-frameworks#22

It's a feature request that should be added to express... Does every feature request goes through that group? They seem less active than this group but will open the same discussion there as well. Thanks

[ghinks]

I agree with @ywarezk that the handling is currently not totally ideal. There is an excellent article on handling async errors in express. It is possible to catch the errors via the error handler

const express = require("express");

const app = express();
const randomOtherMiddleware = ( req, res, next) => {
  console.log("random other middleware");
  next();
};
const errHndlingMiddleWare = ( err, req, res, next ) => {
  console.error(err.message);
  res.status(500).send("unhandled error");
};
const errorCreatingMiddleware = (req, res, next) => {
  setTimeout(() => {
    next(new Error("something happened"));
  }, 500);
};

app.get("*", [errorCreatingMiddleware, randomOtherMiddleware, errHndlingMiddleWare]);

app.listen(3000, function() {
  console.log("we are now listening to port 3000");
});

or via wrapping async calls as here

const app = require('express')();

app.get('*', wrapAsync(async function(req, res) {
  await new Promise(resolve => setTimeout(() => resolve(), 50));
  // Async error!
  throw new Error('woops');
}));

app.use(function(error, req, res, next) {
  // Gets called because of `wrapAsync()`
  res.json({ message: error.message });
});

app.listen(3000);

function wrapAsync(fn) {
  return function(req, res, next) {
    // Make sure to `.catch()` any errors and pass them along to the `next()`
    // middleware in the chain, in this case the error handler.
    fn(req, res, next).catch(next);
  };
}

this is not the ideal resolution you seek, however for others with this issue it does provide a solution so I am putting it into the discussion here.

[ywarezk]

I tried playing a bit with zone.js to grab async errors, and invoke the default error handler. Did the following and it seems to work:

require("zone.js");

const onHandleError = (parentZoneDelegate, currentZone, targetZone, error) => {
    const res = currentZone.get("res");
    res.send("async error activate the default express error handler");
}

const express = require("express");

const app = express();

app.get("*", function(req, res) {
    const middlewareZone = Zone.current.fork({
        properties: {
            req, res
        },
        onHandleError,  
        name: 'express child'              
    });
    middlewareZone.run(() => {
        console.log(Zone.current.name);
        setTimeout(() => {
            throw new Error('something happened');
        }, 1000);
    })
})

app.listen(3000, function() {
    console.log("we are now listening on port 3000");
});

A zone has to be created per request response cycle so we can save the req/res/next in the zone. So when a request enters we can open a zone for that request. I opened the zone inside my middleware for POC but we can do better and open once per request response. The middleware function should run in the zone we create. If exception happens the zone onHandleError catch the exception (async or sync it doesn't matter it will catch all exceptions), can grab the res or next and pop an error. with this there will be no need to doing next(new Error())

[fed135]

Do you have any insights on the overhead from using a full async hook solution ? In my experience it's pretty dreadfull. I'm not against the feature itself, but the ecosystem is simply not ready for it- hence the discussion in the nodejs frameworks working group.

[ywarezk]

Do you have any insights on the overhead from using a full async hook solution ? In my experience it's pretty dreadfull. I'm not against the feature itself, but the ecosystem is simply not ready for it- hence the discussion in the nodejs frameworks working group.

@fed135 you are right. Although the idea seems to me the most elegant and easy to implement with Zone.js (might be even possible to encapsulate the concept in an easy to use middleware, will try to do that), the idea will rise and fall based on performance.

Still need to do some benchmarking on the subject, but based on what I read and my experience with angular and zone, the performance was not great, so I have a feeling that you are right, but will do some further check and publish my results.

[vdeturckheim]

There is currently a work to provide a CLS-like API directly in Node.js core in a performant and memory-safe way (see https://github.com/nodejs/web-server-frameworks/issues/22#issuecomment-592003434) Also, I think the landing of https://github.com/nodejs/node/commit/fd3d02ac32660dfe5edcfa7592576da68780f7e2 should enable us to update the perfs of the domain core module which is basically designed to do this but is now documentation-deprecated.

[anlexN]

why let error handing must be async? i think this step is wrong.

[yz89122]

Do you have any insights on the overhead from using a full async hook solution ? In my experience it's pretty dreadfull. I'm not against the feature itself, but the ecosystem is simply not ready for it- hence the discussion in the nodejs frameworks working group.

Do you have any insights on the overhead from using a full async hook solution ? In my experience it's pretty dreadfull. I'm not against the feature itself, but the ecosystem is simply not ready for it- hence the discussion in the nodejs frameworks working group.

@fed135 you are right. Although the idea seems to me the most elegant and easy to implement with Zone.js (might be even possible to encapsulate the concept in an easy to use middleware, will try to do that), the idea will rise and fall based on performance.

Still need to do some benchmarking on the subject, but based on what I read and my experience with angular and zone, the performance was not great, so I have a feeling that you are right, but will do some further check and publish my results.

How about this change?

And we can rewrite the middleware as

router.use((req, res, next) => {
  // do STH
  return next(); // the return is required to return `Promise<void>` or just `void`
})

And also we can catch the error by throw

router.use(async (req, res, next) => {
  try {
    await next();
  } catch (e) {
    console.error(e);
    res.status(500).end();
    // or
    next(e);
  }
})

This change also allow

router.use(async (req, res, next) => {
  const connection = pool.get();
  res.locals.connection = connection;
  try {
    await next();
  } finally {
    pool.add(connection);
  }
})

BUT, this change require all middleware to change from

router.use((req, res, next) => {
  // do STH
  next();
})

to

router.use((req, res, next) => {
  // do STH
  return next();
})

(e.g. this)