Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions thrown on async providers in Request scope are not handled. #2290

Closed
dynamikus opened this issue May 27, 2019 · 6 comments
Closed
Labels
needs triage This issue has not been looked into

Comments

@dynamikus
Copy link

dynamikus commented May 27, 2019

Bug Report

maybe related to
#1126
#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
@dynamikus dynamikus added the needs triage This issue has not been looked into label May 27, 2019
@dynamikus dynamikus changed the title Expectations thrown on async providers on Request scope are not handled. Exceptions thrown on async providers on Request scope are not handled. May 27, 2019
@dynamikus dynamikus changed the title Exceptions thrown on async providers on Request scope are not handled. Exceptions thrown on async providers in Request scope are not handled. May 28, 2019
@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented May 28, 2019

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

@dynamikus
Copy link
Author

dynamikus commented May 28, 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.

@kamilmysliwiec
Copy link
Member

Let me reopen this and look at the best possible solution

@dynamikus
Copy link
Author

dynamikus commented May 29, 2019

@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.

@kamilmysliwiec
Copy link
Member

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

@lock
Copy link

lock bot commented Nov 6, 2019

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 lock bot locked as resolved and limited conversation to collaborators Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

2 participants