nest icon indicating copy to clipboard operation
nest copied to clipboard

Uncaught EmptyError when proxying after upgrading to Nest.js 8

Open david-golightly-leapyear opened this issue 2 years ago • 10 comments

Regression

Potential Commit/PR that introduced the regression**

Describe the regression

Before, the example code worked flawlessly. After Nest.js 8 upgrade, rxjs throws an uncaught EmptyError ("no elements in sequence"). I can hide this by catching with an ExceptionFilter, but I'd like to know what's going on and whether this usage is still supported/whether I should be doing something differently. Note that none of the given error handlers catch this error. The stack trace is entirely within rxjs code:

        stack: 'Error: \n' +
          '    at _super (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/util/createErrorClass.ts:13:22)\n' +
          '    at new EmptyErrorImpl (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/util/EmptyError.ts:26:3)\n' +
          '    at Object.complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/lastValueFrom.ts:72:18)\n' +
          '    at Object.complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/Subscriber.ts:194:14)\n' +
          '    at SafeSubscriber.Object.<anonymous>.Subscriber._complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/Subscriber.ts:132:24)\n' +
          '    at SafeSubscriber.Object.<anonymous>.Subscriber.complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/Subscriber.ts:106:12)\n' +
          '    at OperatorSubscriber.Object.<anonymous>.Subscriber._complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/Subscriber.ts:132:24)\n' +
          '    at OperatorSubscriber.Object.<anonymous>.Subscriber.complete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/Subscriber.ts:106:12)\n' +
          '    at checkComplete (/Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/operators/switchMap.ts:95:78)\n' +
          '    at /Users/davidgolightly/dev/leapyear/mono/.yarn/cache/rxjs-npm-7.3.0-8a14d1c3d9-e63adb8808.zip/node_modules/rxjs/src/internal/operators/switchMap.ts:118:17',
        name: 'EmptyError',
        message: 'no elements in sequence'

This was originally masked by this error:

    Cannot set headers after they are sent to the client

      at ServerResponse.header (../.yarn/cache/express-npm-4.17.1-6815ee6bf9-d964e9e17a.zip/node_modules/express/lib/response.js:771:10)
      at ServerResponse.json (../.yarn/cache/express-npm-4.17.1-6815ee6bf9-d964e9e17a.zip/node_modules/express/lib/response.js:264:10)
      at ExpressAdapter.reply (../.yarn/__virtual__/@nestjs-platform-express-virtual-3806b0d2c4/0/cache/@nestjs-platform-express-npm-8.0.6-dd80e07a74-c1db29163a.zip/node_modules/@nestjs/platform-express/adapters/express-adapter.js:29:57)
      at ExceptionsHandler.handleUnknownError (../.yarn/unplugged/@nestjs-core-virtual-8150dd869e/node_modules/@nestjs/core/exceptions/base-exception-filter.js:38:24)
      at ExceptionsHandler.catch (../.yarn/unplugged/@nestjs-core-virtual-8150dd869e/node_modules/@nestjs/core/exceptions/base-exception-filter.js:17:25)
      at ExceptionsHandler.next (../.yarn/unplugged/@nestjs-core-virtual-8150dd869e/node_modules/@nestjs/core/exceptions/exceptions-handler.js:16:20)
      at ../.yarn/unplugged/@nestjs-core-virtual-8150dd869e/node_modules/@nestjs/core/router/router-proxy.js:13:35

but with an ExceptionFilter, I was able to catch the original error.

We're using proxy from express-http-proxy to forward data from a backend service (written in Haskell, and not a Nest.js microservice). We tried a couple more manual approaches, such as using Axios, but since we're using a custom payload format, it is a requirement that this Nest.js server proxy requests/responses transparently, without attempting to parse anything in the body and while supporting streaming as necessary, and express-http-proxy "just works" where Axios has been more tricky to figure out.

I'm not even really sure what EmptyError means in this context; we can test that requests are correctly proxied, including headers & response bodies, so I'm not sure what's "empty" here when the functionality otherwise works as intended.

I suspect there's something simple I'm missing; also. Thanks for reviewing!

Input Code

class HttpProxyInterceptor implements NestInterceptor {
  intercept(ctx: ContextWithSpan, next: CallHandler): Observable<unknown> {
      const http = ctx.switchToHttp()
      const req = http.getRequest()

      const res = http.getResponse()
      const body = getRawBody(req)

      return next.handle().pipe(
        defaultIfEmpty(''), // added for debugging, does not help
        switchMap((proxyOptions: ProxyOptions) => {
          const proxyHost = proxyOptions.url ?? host
          if (!proxyHost) {
            throw new Error('No host to proxy')
          }

          return new Observable((subscriber) => {
            const handleErrors = (e: unknown) => {
              console.error(e) // added for debugging, does not get called
              subscriber.error(`HttpProxy error: ${e}`)
            }

            res.on('finish', () => subscriber.complete())

            proxy(proxyHost, {
              ...options,
              ...proxyOptions,
              proxyErrorHandler: handleProxyErrors,
              proxyReqBodyDecorator: () => body,
            })(req, res, handleErrors)
          })
        }),
        catchError((e) => {
          return of('') // added for debugging, does not help
        })
      )
  }
}

Expected behavior/code

No error.

Environment


Nest version: 7.6.18 -> 8.0.6

For Tooling issues:
- Node version: v16.8.0  
- Platform:  Mac 

Others:

Yarn berry

Please provide a minimum reproduction repository.

jmcdo29 avatar Sep 22 '21 20:09 jmcdo29

If I had to guess, this comes from the new use of lastValueFrom in RxJS v7. If there's no values in the Observable, then it will error out. You can see this functionality in this stackblitz. toPromise, what used to be used instead of lastValueFrom would just return undefined and let the execution move on. It would seem that the way the proxy response is handled is creating a problem

jmcdo29 avatar Sep 22 '21 20:09 jmcdo29

@jmcdo29 Thanks for the quick reply! Here's a somewhat minimal repro, excised from our production code:

https://github.com/davidgoli/nestjs-empty-error-repro

Could it partly be the problem that express-http-proxy is writing the response directly to response, and not to the observable?

It looks like this is working if I simply change:

res.on('finish', () => subscriber.complete())

to:

res.on('finish', () => {
  subscriber.next();
  subscriber.complete();
});

That could definitely be an issue as well. Because the proxy middleware is handling the response, when it gets back to Nest, Nest tries to send a response too, and that "Cannot set headers after they are sent to the client" gets triggered

jmcdo29 avatar Sep 22 '21 21:09 jmcdo29

It seems that you already found the solution to your issue @david-golightly-leapyear so I'm assuming we can close this thread(?). As @jmcdo29 said, lastValueFrom errors out if there are no values in the Observable (hence the subscriber.next() resolves the issue).

kamilmysliwiec avatar Oct 04 '21 07:10 kamilmysliwiec

I have a workaround for the issue, but it feels like a hack. Is lastValueFrom really the correct invocation, if it's possible there are no values?

Is lastValueFrom really the correct invocation, if it's possible there are no values?

In a regular Nest app, there's no possibility that no values would be emitted to the stream (even if the method returns undefined or an empty string).

kamilmysliwiec avatar Oct 05 '21 06:10 kamilmysliwiec

@kamilmysliwiec is proxying to a backend service not a use case handled by a "regular" Nest app? Can you tell me whether the implementation I have above is supported, or whether there is a more preferred approach? There might be a documentation change here if there is.

We also observed this behavior, and fixed it by ensuring that the returned Observable emits at least one value (using startWith). Our use case is Server-sent events.

panuhorsmalahti avatar Apr 08 '22 11:04 panuhorsmalahti

https://github.com/nestjs/nest/pull/8802

kamilmysliwiec avatar Jun 26 '23 10:06 kamilmysliwiec