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

Uncaught EmptyError when proxying after upgrading to Nest.js 8 #8111

Closed
david-golightly-leapyear opened this issue Sep 22, 2021 · 11 comments
Closed
Labels
needs triage This issue has not been looked into type: bug 😭

Comments

@david-golightly-leapyear

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
@david-golightly-leapyear david-golightly-leapyear added needs triage This issue has not been looked into type: bug 😭 labels Sep 22, 2021
@jmcdo29
Copy link
Member

jmcdo29 commented Sep 22, 2021

Please provide a minimum reproduction repository.

@jmcdo29
Copy link
Member

jmcdo29 commented Sep 22, 2021

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

@david-golightly-leapyear
Copy link
Author

@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

@david-golightly-leapyear
Copy link
Author

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();
});

@jmcdo29
Copy link
Member

jmcdo29 commented Sep 22, 2021

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

@kamilmysliwiec
Copy link
Member

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

@david-golightly-leapyear
Copy link
Author

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?

@kamilmysliwiec
Copy link
Member

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

@david-golightly-leapyear
Copy link
Author

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

@panuhorsmalahti
Copy link

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.

@kamilmysliwiec
Copy link
Member

#8802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: bug 😭
Projects
None yet
Development

No branches or pull requests

4 participants