Refactor error handling in grpcwebclientbase #858
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
More fixes on how various callbacks are being handled in GrpcWebClientBase.
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 aClientReadableStream
, where user can subscribe to various callbacks likestatus
/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 ofrpcCall()
to be a specialized version ofClientReadableStream
that will disable the.on('data')
and.on('error')
callbacks even if added.In an ongoing effort to clean up how errors are handled / returned in the
GrpcWebClientReadableStream
base class, I am rerouting ALL errors to ahandleError_
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 callhandleError_
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.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_()
.Added a lot of tests.
Summary (for callback-based calls):
(err, resp) => ...
callback.on('status')
callback.on('error')
callbackresp
returnederr
returnederr
returned.on('data')
callback.on('status')
callback.on('error')
callback