Unhandled promise rejection in interceptor

omerd-cyera commented 1 week ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

Receiving error events in the interceptor observer before all interceptors have been called triggers an unhandled promise rejection. I have attached a codesandbox link that shows this happening. Note that to get this behavior, we have to use something like rxjss merge.

I ran into this issue when creating an interceptor that returns something like merge(next.handle(), interval()), and the interval throws before all the next.handle finish.

Minimum reproduction code

Steps to reproduce

It should trigger automatically when the integrated browser opens

Expected behavior

I would expect all subsequent exceptions after the first one to be either passed to the exception handling mechanism, or ignored.

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[X] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[ ] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] Other (see below)

Other package

No response

NestJS version

10.3.2

Packages versions

{
  "name": "nest-typescript-starter",
  "private": true,
  "version": "1.0.0",
  "description": "Nest TypeScript starter repository",
  "license": "MIT",
  "scripts": {
    "build": "nest build",
    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
    "start": "nest start",
    "start:dev": "nest start --watch",
    "start:debug": "nest start --debug --watch",
    "start:prod": "node dist/main",
    "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix",
    "test": "jest",
    "test:watch": "jest --watch",
    "test:cov": "jest --coverage",
    "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/jest/bin/jest --runInBand",
    "test:e2e": "jest --config ./test/jest-e2e.json"
  },
  "dependencies": {
    "@nestjs/common": "^10.3.2",
    "@nestjs/core": "^10.3.2",
    "@nestjs/platform-express": "^10.3.2",
    "reflect-metadata": "^0.2.1",
    "rxjs": "^7.8.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^10.3.1",
    "@nestjs/schematics": "^10.1.0",
    "@nestjs/testing": "^10.3.2",
    "@swc/cli": "^0.3.9",
    "@swc/core": "^1.4.0",
    "@types/express": "^4.17.21",
    "@types/jest": "^29.5.12",
    "@types/node": "^20.11.16",
    "@types/supertest": "^6.0.2",
    "@typescript-eslint/eslint-plugin": "^6.21.0",
    "@typescript-eslint/parser": "^6.21.0",
    "eslint": "^8.56.0",
    "eslint-config-prettier": "^9.1.0",
    "eslint-plugin-prettier": "^5.1.3",
    "jest": "^29.7.0",
    "prettier": "^3.2.5",
    "source-map-support": "^0.5.21",
    "supertest": "^6.3.4",
    "ts-jest": "^29.1.2",
    "ts-loader": "^9.5.1",
    "ts-node": "^10.9.2",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.3.3"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".*\\.spec\\.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "collectCoverageFrom": [
      "**/*.(t|j)s"
    ],
    "coverageDirectory": "../coverage",
    "testEnvironment": "node"
  }
}

Node.js version

v20.9.0

In which operating systems have you tested?

[ ] macOS
[ ] Windows
[X] Linux

Other

The package.json and node version I specified are the ones in codesandbox. But it happens to me on node 18.17.0 and 10.3.0 too.

I think I know what the issue is. I managed to resolve it locally by patching packages/core/interceptors/interceptors-consumer.ts:33

I replaced

    const nextFn = async (i = 0) => {
      if (i >= interceptors.length) {
        return defer(AsyncResource.bind(() => this.transformDeferred(next)));
      }
      const handler: CallHandler = {
        handle: () => fromPromise(nextFn(i + 1)).pipe(mergeAll()),
      };
      return interceptors[i].intercept(context, handler);
    };

with

    const nextFn = async (i = 0) => {
      if (i >= interceptors.length) {
        return defer(AsyncResource.bind(() => this.transformDeferred(next)));
      }
      const handler: CallHandler = {
        handle: () => defer(() => from(nextFn(i + 1))).pipe(mergeAll()),
      };
      return interceptors[i].intercept(context, handler);
    };

When looking at this stackOverflow question: https://stackoverflow.com/questions/39319279/convert-promise-to-observable I followed coderrr22's warning on the accepted answer

Please be careful with this solution, Because it may result in an unhandled promise rejection which can break your code! If you are using from operator you must handle your promise rejection in the promise within the from operator(using .catch()), otherwise the promise may execute and throw an error, while its wrapping observable hasn't been subscribed yet, and if your error handling is piped to the observable it will not catch the error ! (I prefer using defer instead and handling the errors centrally from the observable).

kamilmysliwiec commented 1 week ago

Would you like to create a PR for this issue?

omerd-cyera commented 1 week ago

Would you like to create a PR for this issue?

@kamilmysliwiec sure

sheepster1 commented 1 week ago

@kamilmysliwiec Created the PR from another account

kamilmysliwiec commented 1 week ago

Let's track this here https://github.com/nestjs/nest/pull/13762

nestjs / nest