jeffijoe / awilix-express

Awilix helpers/middleware for Express
MIT License
114 stars 7 forks source link

unhandledRejection when returning promises from routes #19

Closed sbarry157 closed 4 years ago

sbarry157 commented 4 years ago

We would like to start using awilix-express moving forward, but promise rejections are not handled as we expected. Using express-promise-router, our pattern is this:

all routes return promises When an error happens, throw immediately. register an error handler (app.use) to inspect the error and return the appropriate response.

I noticed #15 relates to this issue.

But in looking at the code, I see that the error is re-thrown after calling next. I think this is causing the issue.

function asyncErrorWrapper(
  fn: (...args: any[]) => any
): (...args: any[]) => any {
  return function asyncWrappedMiddleware(
    req: Request,
    res: Response,
    next: NextFunction
  ) {
    const returnValue = fn(req, res, next)

    if (
      returnValue &&
      returnValue.catch &&
      typeof returnValue.catch === 'function'
    ) {
      return returnValue.catch((err: any) => {
        next(err)
        throw err // <-- Does this need to be here?
      })
    } else {
      return returnValue
    }
  }
}

Looking at express-promise-router, they don't re-throw in this situation:

// Call the route
var ret = handler.apply(null, args);

// If it doesn't return a promise, we exit.
if (!isPromise(ret)) {
    return;
}

// Since we got a promise, we handle calling next
Promise.resolve(ret).then(
    function(d) {
        if (d === 'next') {
            next();
        } else if (d === 'route') {
            next('route');
        }
    },
    function(err) {
        if (!err) {
            err = new Error('returned promise was rejected but did not have a reason');
        }
        next(err); // <-- no rethrow
    }
);

Maybe I'm not using the library correctly?

jeffijoe commented 4 years ago

I think you're right, it shouldn't rethrow it.

jeffijoe commented 4 years ago

@sbarry157 released as 3.0.0, I also bumped the supported node version range.

sbarry157 commented 4 years ago

@jeffijoe excellent. Thanks for the quick resolution :)

jeffijoe commented 4 years ago

No problem! I'd suggest using Koa though. :)