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

RPC: Thrown Exceptions are reported as errors wrapped in success #290

Closed
nicolaslt opened this issue Dec 7, 2017 · 14 comments

Comments

@nicolaslt
Copy link
Contributor

commented Dec 7, 2017

I'm submitting a bug or doc issue


[ ] Regression 
[x] Bug report
[ ] Feature request
[x] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Using RPC with transformToObservable as recommended on the Custom Transport documentation. Exceptions handled by RpcExceptionsHandler are attempted to be sent as a success.

Returned Observable from transformToObservable resolves to another Observable being the one created by RpcExceptionsHandler.

Expected behavior

Throwing in the route handler should return an ErrorObservable, as opposed to a success Observable wrapping an error one.

I suspect RpcExceptionsHandler should just throw an Error instead of returning an instance of Observable.throw. This will in turn make the promise in RpcProxy.create correctly fail, which will in turn make transformToObservable behave as expected.

Environment


Nest version: 4.4.1


Other:
This problem exist regardless of the Error thrown. It misbehaves with instances of RpcException as well as plain Error

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 10, 2017

Hi @nicolaslt,
I can't reproduce this issue. What exactly are you trying to achieve? How can I test this behaviour? 🙂

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

Here's a typescript example that reproduces the problem. Hopefully it will make more sense. :)

import { Server, CustomTransportStrategy } from '@nestjs/microservices'
import { NestFactory } from '@nestjs/core'
import { Controller, Module } from '@nestjs/common'
import { MessagePattern } from '@nestjs/microservices'


class CustomStrategy extends Server implements CustomTransportStrategy {
  private channel: any = null
  private server: any = null

  constructor() {
    super();
  }

  async listen(cb: () => void) {
    cb()
  }

  close() {
    this.channel && this.channel.close();
    this.server && this.server.close();
  }

  handleMessage(message: any) {
    const handler = this.getHandlers()['"throw"']

    //Important to reproduce the bug: transformToObservable needs to be called with a Promise (hence why no `await` statement)
    const response$ = this.transformToObservable(handler(message));
    return response$
  }
}

@Controller()
export class MathController {
  @MessagePattern('throw')

  //This function is designed to throw for the purpose of the example.
  //Important to reproduce the bug: this function needs to return a Promise
  async sum(data: Array<number>) {
    if(data != [1]) {
      throw new Error("I failed")
    }
    return (data || []).reduce((a: number, b: number) => a + b);
  }
}

@Module({
  controllers: [MathController]
})
class ApplicationModule {
}

(async () => {
  const strategy = new CustomStrategy()
  const app = await NestFactory.createMicroservice(ApplicationModule, { strategy });
  app.listen(() => {
    strategy.handleMessage('stuff')
      .subscribe({
        next (result:any) {
          //Execution goes there, and result is an instance of ErrorObservable
          console.log("Failure disguised as success")
          console.log(result)
        },
        error(error) {
          console.log('Correct failure')
          console.log(error)
          //Execution should go there, with error being of type {status, message}
        }
      })
  })
})()

Output is:

Failure disguised as success
ErrorObservable {
  _isScalar: false,
  error: { status: 'error', message: 'Internal server error' },
  scheduler: undefined }
@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

Why you're not awaiting handler(message)? (as in the example)

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

I'm not in the context of an async function, and the thing I'm using expects an Observable, not a promise.

transformToObservable supports promises, so I think it's reasonable to expect this promise to work without waiting for it.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

@nicolaslt yeah, but as I remember all handlers are wrapped into the evaluation context function, which makes them async functions by default. It means that even if you're handler returns observable and you won't await it, this transformToObservable will treat it as a Promise, instead of Observable.

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2017

@kamilmysliwiec Yes. I think we don't understand each other here.

The bug I'm reporting is that if an error is handled by RpcExceptionsHandler the promise returned by the evaluation context function will succeed with its success value being an error. That is, in my opinion, a violation of the Promise usage.

Put in other words, in the context of an error being throw by the handler, I expect the following to happen:

try {
  const result = await handler(messageObj.data)
} catch(e) {
  //here is the error 
}

But right now this is what happens

try {
  const result = await handler(messageObj.data)
} catch(e) {
  //the error is not here, it's in result.
}```
@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

Could you show here how the result variable output looks like?

try {
  const result = await handler(messageObj.data)
} catch(e) {
  //the error is not here, it's in result.
}

In above case ^

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

I took the liberty of logging what you asked for, but also what the example provided above logs.

Code:

try {
  const result = await strategy.handleMessage('stuff')
  console.log('result', result)
  result.subscribe({
    next (res:any) {
      console.log("Failure disguised as success")
      console.log(res)
    },
    error(error) {
      console.log('Correct failure')
      console.log(error)
    }
  })
} catch(e) {
 console.error('Caught an error')
}

Logs

result PromiseObservable {
  _isScalar: false,
  promise: Promise { <pending> },
  scheduler: undefined }
Failure disguised as success
ErrorObservable {
  _isScalar: false,
  error: { status: 'error', message: 'Internal server error' },
  scheduler: undefined }
@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

You can get it from there https://github.com/nicolaslt/nest/tree/rpc-error-success
And run it with npm run build && node node_modules/@nestjs/common/test-error.js

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 30, 2017

@nicolaslt have you still encountered this issue after the latest update?

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

Yes, I still have the issue after merging master (4d4f7a448ce472e939cf835b2beb6e12b3495f3d) into my branch.
I realised that I had forgotten to add the test-error.ts file in that branch, it's now in there, you should be able to run the program and get those logs on your own machine now.

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

Hm, wait I think I didn't merge master correctly. That commit is from 20 days ago. I'll reply once merged correctly and re-tested.

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

Alright, updated to 5ed8c5980b3c78581b74167be76cb0c2a78dc86d.
Still has the problem.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

In 4.6.0 (will be published today/tomorrow) code that you have provided should be changed to:

async handleMessage(message: any) {
    const handler = this.getHandlers()['"throw"']
    const response$ = this.transformToObservable(await handler(message));
    return response$;
}

Thanks to that wrapped async stuff will be executed. Using following code:

(await strategy.handleMessage('stuff'))
      .subscribe({
        next (result:any) {
          //Execution goes there, and result is an instance of ErrorObservable
          console.log("Failure disguised as success")
          console.log(result)
        },
        error(error) {
          console.log('Correct failure')
          console.log(error)
          //Execution should go there, with error being of type {status, message}
        }
      })

Exception will go to error() handler (as a correct failure), exception will be logged, but error will be:

{ status: 'error', message: 'Internal server error' }

It's an expected behaviour since all exceptions are handled by global exception filter. In order to change this, you can create your own global exception filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.