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

Refactor error handling in grpcwebclientbase #858

Merged

Conversation

stanley-cheung
Copy link
Collaborator

@stanley-cheung stanley-cheung commented Jun 20, 2020

More fixes on how various callbacks are being handled in GrpcWebClientBase.

  1. Unary callback executed multiple times on gRPC error #632: in the callback-based API, the return type of a unary call (i.e. rpcCall()) is currently a ClientReadableStream, where user can subscribe to various callbacks like status/error/end/metadata via the .on() API (i.e. EventEmitter interface). However, there is currently a discrepancy between grpc-web and grpc-node. In grpc-node, such a return value will not react to .on('error') because the error is already being returned by the main (err, resp) => ... calback. Currently in grpc-web, both the main (err, resp) => ... callback and the .on('error') callback will fire on a single error, which is confusing. This change attempts to align the behavior with grpc-node by modifying the return value of rpcCall() to be a specialized version of ClientReadableStream that will disable the .on('data') and .on('error') callbacks even if added.

  2. In an ongoing effort to clean up how errors are handled / returned in the GrpcWebClientReadableStream base class, I am rerouting ALL errors to a handleError_ function for now. This also aligns with how grpc-node handles these errors - in the streaming case, each error will trigger both an .on('status') callback and an .on('error') callback. So as a first effort, let's always call handleError_ in this class. Coz currently, some errors trigger an .on('status'), some errors trigger an .on('error') and there's no reason for that discrepancy.

  3. grpc-web should warn when the Content-Type of a response is not supported #466: Now with the above, we can easily fix issues like this grpc-web should warn when the Content-Type of a response is not supported #466. Everything should be an error being triaged by handleError_().

  4. Added a lot of tests.

Summary (for callback-based calls):

  1. Unary calls
scenario main (err, resp) => ... callback .on('status') callback .on('error') callback
proper response + grpc-status: OK resp returned
proper response + grpc-status: non-OK err returned
any other error scenario (network error, deadline exceeded, unimplemented, etc) err returned
  1. Server streaming calls
scenario .on('data') callback .on('status') callback .on('error') callback
proper response + grpc-status: OK
proper response + grpc-status: non-OK
any other error scenario (network error, deadline exceeded, unimplemented, etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant