Nest: Exceptions thrown on async providers in Request scope are not handled.

Created on 27 May 2019  路  6Comments  路  Source: nestjs/nest

Bug Report

maybe related to
https://github.com/nestjs/nest/issues/1126
https://github.com/nestjs/nest/issues/761

Current behavior

When routes with async providers are used request crashes and client never gets any response.
Error shows up as UnhandledPromiseRejectionWarning in console

Input Code

replication repo
https://github.com/dynamikus/replicate-nestjs-request-scope-usefactory-error

{
  provide: SomeService,
  scope: Scope.REQUEST,
  useFactory: async () => {
    throw new Error('Boom');
   return new Somervice();
  },
}

Expected behavior

Errors thrown from async provider should buble up to the global exception handler. So we can gracefully respond to the client.

Possible Solution


Sorry tried to be able to figure out for a solution but I am not familiar with nestjs core.
Error bubbles up to here
https://github.com/nestjs/nest/blob/master/packages/core/router/router-explorer.ts#L195
We need to wrap with try and catch the handler method and apply the exception filter (which I am not sure what the correct nestjs to handle this. :( )
I monkey patched a solution but I am not sure if this is the proper nestjs way to handle this. Let me know
if this approach is Ok and I will open a pr. 

    if (isRequestScoped) {
      const handler = async <TRequest, TResponse>(
        req: TRequest,
        res: TResponse,
        next: () => void,
      ) => {
        const contextId = createContextId();
        this.registerRequestProvider(req, contextId);
        try {
          const contextInstance = await this.injector.loadPerContext(
            instance,
            module,
            collection,
            contextId,
          );
          this.createCallbackProxy(
            contextInstance,
            contextInstance[methodName],
            methodName,
            moduleKey,
            requestMethod,
            contextId,
            instanceWrapper.id,
          )(req, res, next);

          paths.forEach(path => {
            const fullPath = stripSlash(basePath) + path;
            routerMethod(stripSlash(fullPath) || '/', handler);
          });
        } catch (error) {
          const host = new ExecutionContextHost([req, res]);
          const exceptionFilter = this.exceptionsFilter.create(
            instance,
            () => {},
            methodName,
            contextId,
            instanceWrapper.id,
          );
          exceptionFilter.next(error, host);
        }
      };

      return;
    }

Environment



Nest version: 6.2.4



For Tooling issues:



    

  • Node version: 12.0.1
    • 

  • Platform:  Windows
    •
needs triage
Source
picture dynamikus

Most helpful comment

Let me reopen this and look at the best possible solution

picture kamilmysliwiec on 29 May 2019
🎉2

All 6 comments

You cannot apply exception filters to the custom providers. Your async providers should never throw exceptions, just put this logic elsewhere

kamilmysliwiec picture kamilmysliwiec on 28 May 2019

Even for providers in Request Scope :( . I understand the service resolution on application start up but on request context, service resolution should be able to gracefully fail.

dynamikus picture dynamikus on 28 May 2019

Let me reopen this and look at the best possible solution

kamilmysliwiec picture kamilmysliwiec on 29 May 2019
🎉2

@kamilmysliwiec thank you that would be great, this simplifies a lot use cases when you need to decide on run time (based on user/feature etc) what service to pass to the controller.

dynamikus picture dynamikus on 29 May 2019

Fixed in 6.4.0. Keep in mind that these exceptions will be catchable only by statically scoped exception filters :)

kamilmysliwiec picture kamilmysliwiec on 1 Jul 2019
👍1

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock[bot] picture lock[bot] on 6 Nov 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

raydirty picture raydirty  路  37Comments
j picture j  路  32Comments
kamilmysliwiec picture kamilmysliwiec  路  178Comments
hypeofpipe picture hypeofpipe  路  27Comments
MonsieurMan picture MonsieurMan  路  28Comments