fix(grpc-js): preserve original error code when the handler of a readable stream throws an error

[mad-it]

When writing a small decorator for allowing developers write their code with throwing errors I stumbled upon an issue.

It looks like the grpc-js library overwrites the error code of an error being thrown even if code is present. An example is the following:

const streamHandlerDecorator = <RequestType, ResponseType>(
  handler: (
    sessionId: string,
    stream: ServerWritableStream<RequestType, ResponseType>,
  ) => Promise<void>,
): ((
  stream: ServerWritableStream<RequestType, ResponseType>,
) => Promise<void>) => async (
  stream: ServerWritableStream<RequestType, ResponseType>,
): Promise<void> => {
  try {
    await handler(stream);
  } catch (error) {
    const maybeConvertedError =
      error instanceof GrpcError
        ? error
        : new GrpcError('An unknown error ocurred.', status.INTERNAL);

    stream.emit('error', maybeConvertedError);
  }
};

GrpcError is basically a wrapper around the ServiceError type exported by grpc-js.

This snippet of code works perfectly for ServerWritableStream, but not for ServerDuplexStream or ServerReadableStream.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Algin Maduro (5e42be1, aaee068)

[murgatroid99]

The real problem here is that readable.push is being called inside of the try...catch block. It is only supposed to catch errors from the this.deserializeMessage call, not errors from the application. The application should send errors using call.emit('error', error), just like this code does.

[mad-it]

The real problem here is that readable.push is being called inside of the try...catch block. It is only supposed to catch errors from the this.deserializeMessage call, not errors from the application.

I just made a small attempt locally for changing the try/catch block to only catch errors for this.deserializeMessage. Everything went crazy in my test suite. I am not really comfortable making such a change if this is what you meant. At the end of the day if you would like to support this you would need 2 try/catch block.

1. for this.deserializeMessage
2. for readable.push as this can throw an error from the application level. Otherwise errors throw in the application with bork the runtime if uncaught.

☝️ does not seem like a logical way to move forward.

The application should send errors using call.emit('error', error), just like this code does.

I found this a bit confusing. Are you implying that its not possible to write a decorator for error handling when it comes to streams with an underlying readable stream (ServerReadableStream/ServerDuplexStream) ?

Let me try to explain what I'm trying to achieve. Maybe that might shed some light.
From an application level its kind of annoying to have to write code in the following manner:

if(!isValidRequest(data)){
  stream.emit('error', new GrpcError(xxxxxx));

  return;
}

doSomething(data);

As you can see from an application perspective its not ideal to have to write a return statement every time you need to emit an error. Normally one would throw an error which would end the execution flow of the application.

[murgatroid99]

The fundamental problem here is that we can't guarantee that application operations happen in the same call stack as the original handler function, so thrown errors won't bubble up to where you want them to go. Any async operation in the handler breaks that. So gRPC can make a best-effort attempt to catch what errors it can, but the only consistent way to return an error from a stream is by using call.emit('error', error).

I want to note that when I said "The application should send errors using call.emit", I included the decorated handler in "the application". So if the decorator is responsible for handling all errors, and it handles them using call.emit, then I consider that to be correct behavior.

However, I think that the problem you were having with the decorator is exactly this call stack/async context problem. The data event callbacks run in a completely different call stack than the original handler did, so errors thrown in that context do not bubble up to be handled by the decorator. Instead they bubble up through this try...catch block this PR would modify.

I think that your modification does not actually make your decorator work. Instead, it gives that try...catch block behavior that is functionally indistinguishable from your decorator's behavior. But that won't help if for example you perform some async operation in the data event callback, and throw in that async operation's callback.

Overall, I would actually be OK with accepting this change. It preserves the same behavior when handling deserialization errors and continues the pattern of best-effort application error handling that appears in other parts of the code.