From a8416b5e7aa7d5a58772f3405e196440c918776e Mon Sep 17 00:00:00 2001 From: Stanley Cheung Date: Sat, 20 Jun 2020 00:47:31 -0700 Subject: [PATCH] Refactor error handling in grpcwebclientbase --- javascript/net/grpc/web/BUILD.bazel | 1 + javascript/net/grpc/web/grpcwebclientbase.js | 41 +++- .../grpc/web/grpcwebclientreadablestream.js | 47 ++-- .../examples/echo/commonjs-example/client.js | 8 - net/grpc/gateway/examples/echo/echoapp.js | 8 +- net/grpc/gateway/examples/echo/echotest.html | 7 - packages/grpc-web/test/generated_code_test.js | 216 ++++++++++++++---- 7 files changed, 248 insertions(+), 80 deletions(-) diff --git a/javascript/net/grpc/web/BUILD.bazel b/javascript/net/grpc/web/BUILD.bazel index 12e48ced..04f0ff55 100644 --- a/javascript/net/grpc/web/BUILD.bazel +++ b/javascript/net/grpc/web/BUILD.bazel @@ -121,6 +121,7 @@ closure_js_library( visibility = ["//visibility:public"], deps = [ ":clientreadablestream", + ":error", ":generictransportinterface", ":grpcwebstreamparser", ":status", diff --git a/javascript/net/grpc/web/grpcwebclientbase.js b/javascript/net/grpc/web/grpcwebclientbase.js index 2128fb69..e42674a3 100644 --- a/javascript/net/grpc/web/grpcwebclientbase.js +++ b/javascript/net/grpc/web/grpcwebclientbase.js @@ -96,6 +96,45 @@ class GrpcWebClientBase { * @export */ rpcCall(method, requestMessage, metadata, methodDescriptor, callback) { + /** + * @implements {ClientReadableStream} + */ + class ClientUnaryCallImpl { + /** + * @param {!ClientReadableStream} stream + * @template RESPONSE + */ + constructor(stream) { + this.stream = stream; + } + + /** + * @override + */ + on(eventType, callback) { + if (eventType == 'data' || eventType == 'error') { + // unary call responses and errors should be handled by the main + // (err, resp) => ... callback + return this; + } + return this.stream.on(eventType, callback); + } + + /** + * @override + */ + removeListener(eventType, callback) { + return this.stream.removeListener(eventType, callback); + } + + /** + * @override + */ + cancel() { + this.stream.cancel(); + } + } + methodDescriptor = AbstractClientBase.ensureMethodDescriptor( method, requestMessage, MethodType.UNARY, methodDescriptor); var hostname = AbstractClientBase.getHostname(method, methodDescriptor); @@ -105,7 +144,7 @@ class GrpcWebClientBase { var stream = /** @type {!ClientReadableStream} */ (invoker.call( this, methodDescriptor.createRequest(requestMessage, metadata))); GrpcWebClientBase.setCallback_(stream, callback, false); - return stream; + return new ClientUnaryCallImpl(stream); } /** diff --git a/javascript/net/grpc/web/grpcwebclientreadablestream.js b/javascript/net/grpc/web/grpcwebclientreadablestream.js index 2807706b..eee28bba 100644 --- a/javascript/net/grpc/web/grpcwebclientreadablestream.js +++ b/javascript/net/grpc/web/grpcwebclientreadablestream.js @@ -34,6 +34,7 @@ goog.module.declareLegacyNamespace(); const ClientReadableStream = goog.require('grpc.web.ClientReadableStream'); const ErrorCode = goog.require('goog.net.ErrorCode'); const EventType = goog.require('goog.net.EventType'); +const GrpcWebError = goog.require('grpc.web.Error'); const GrpcWebStreamParser = goog.require('grpc.web.GrpcWebStreamParser'); const StatusCode = goog.require('grpc.web.StatusCode'); const XhrIo = goog.require('goog.net.XhrIo'); @@ -103,7 +104,7 @@ class GrpcWebClientReadableStream { /** * @const * @private - * @type {!Array} The list of error callbacks + * @type {!Array} The list of error callbacks */ this.onErrorCallbacks_ = []; @@ -150,6 +151,11 @@ class GrpcWebClientReadableStream { var byteSource = new Uint8Array( /** @type {!ArrayBuffer} */ (self.xhr_.getResponse())); } else { + self.handleError_({ + code: StatusCode.UNKNOWN, + message: 'Unknown Content-type received.', + metadata: {}, + }); return; } var messages = self.parser_.parse(byteSource); @@ -184,11 +190,11 @@ class GrpcWebClientReadableStream { grpcStatusMessage = trailers[GRPC_STATUS_MESSAGE]; delete trailers[GRPC_STATUS_MESSAGE]; } - self.sendStatusCallbacks_(/** @type {!Status} */ ({ + self.handleError_({ code: Number(grpcStatusCode), - details: grpcStatusMessage, + message: decodeURIComponent(grpcStatusMessage), metadata: trailers, - })); + }); } } } @@ -227,9 +233,10 @@ class GrpcWebClientReadableStream { if (grpcStatusCode == StatusCode.ABORTED && self.aborted_) { return; } - self.sendErrorCallbacks_({ + self.handleError_({ code: grpcStatusCode, - message: ErrorCode.getDebugMessage(lastErrorCode) + message: ErrorCode.getDebugMessage(lastErrorCode), + metadata: {}, }); return; } @@ -243,20 +250,13 @@ class GrpcWebClientReadableStream { grpcStatusMessage = self.xhr_.getResponseHeader(GRPC_STATUS_MESSAGE); } if (Number(grpcStatusCode) != StatusCode.OK) { - self.sendErrorCallbacks_({ + self.handleError_({ code: Number(grpcStatusCode), message: grpcStatusMessage, metadata: responseHeaders }); errorEmitted = true; } - if (!errorEmitted) { - self.sendStatusCallbacks_(/** @type {!Status} */ ({ - code: Number(grpcStatusCode), - details: grpcStatusMessage, - metadata: responseHeaders - })); - } } if (!errorEmitted) { @@ -353,6 +353,23 @@ class GrpcWebClientReadableStream { return headers; } + /** + * A central place to handle errors + * + * @private + * @param {!GrpcWebError} error The error object + */ + handleError_(error) { + if (error.code != StatusCode.OK) { + this.sendErrorCallbacks_(error); + } + this.sendStatusCallbacks_(/** @type {!Status} */ ({ + code: error.code, + details: decodeURIComponent(error.message || ''), + metadata: error.metadata + })); + } + /** * @private * @param {!RESPONSE} data The data to send back @@ -385,7 +402,7 @@ class GrpcWebClientReadableStream { /** * @private - * @param {?} error The error to send back + * @param {!GrpcWebError} error The error to send back */ sendErrorCallbacks_(error) { for (var i = 0; i < this.onErrorCallbacks_.length; i++) { diff --git a/net/grpc/gateway/examples/echo/commonjs-example/client.js b/net/grpc/gateway/examples/echo/commonjs-example/client.js index 7716004e..e87ba9e5 100644 --- a/net/grpc/gateway/examples/echo/commonjs-example/client.js +++ b/net/grpc/gateway/examples/echo/commonjs-example/client.js @@ -64,14 +64,6 @@ var echoApp = new EchoApp( { EchoRequest: EchoRequest, ServerStreamingEchoRequest: ServerStreamingEchoRequest - }, - { - checkGrpcStatusCode: function(status) { - if (status.code != grpc.web.StatusCode.OK) { - EchoApp.addRightMessage('Error code: '+status.code+' "'+ - status.details+'"'); - } - } } ); diff --git a/net/grpc/gateway/examples/echo/echoapp.js b/net/grpc/gateway/examples/echo/echoapp.js index d0762922..1f6906a3 100644 --- a/net/grpc/gateway/examples/echo/echoapp.js +++ b/net/grpc/gateway/examples/echo/echoapp.js @@ -21,12 +21,10 @@ const echoapp = {}; /** * @param {Object} echoService * @param {Object} ctors - * @param {Object} handlers */ -echoapp.EchoApp = function(echoService, ctors, handlers) { +echoapp.EchoApp = function(echoService, ctors) { this.echoService = echoService; this.ctors = ctors; - this.handlers = handlers; }; echoapp.EchoApp.INTERVAL = 500; // ms @@ -64,7 +62,6 @@ echoapp.EchoApp.prototype.echo = function(msg) { echoapp.EchoApp.addLeftMessage(msg); var unaryRequest = new this.ctors.EchoRequest(); unaryRequest.setMessage(msg); - var self = this; var call = this.echoService.echo(unaryRequest, {"custom-header-1": "value1"}, function(err, response) { @@ -78,7 +75,6 @@ echoapp.EchoApp.prototype.echo = function(msg) { } }); call.on('status', function(status) { - self.handlers.checkGrpcStatusCode(status); if (status.metadata) { console.log("Received metadata"); console.log(status.metadata); @@ -118,12 +114,10 @@ echoapp.EchoApp.prototype.repeatEcho = function(msg, count) { var stream = this.echoService.serverStreamingEcho( streamRequest, {"custom-header-1": "value1"}); - var self = this; stream.on('data', function(response) { echoapp.EchoApp.addRightMessage(response.getMessage()); }); stream.on('status', function(status) { - self.handlers.checkGrpcStatusCode(status); if (status.metadata) { console.log("Received metadata"); console.log(status.metadata); diff --git a/net/grpc/gateway/examples/echo/echotest.html b/net/grpc/gateway/examples/echo/echotest.html index aca97e63..2809d746 100644 --- a/net/grpc/gateway/examples/echo/echotest.html +++ b/net/grpc/gateway/examples/echo/echotest.html @@ -32,13 +32,6 @@ { EchoRequest: proto.grpc.gateway.testing.EchoRequest, ServerStreamingEchoRequest: proto.grpc.gateway.testing.ServerStreamingEchoRequest - }, - { - checkGrpcStatusCode: function(status) { - if (status.code != grpc.web.StatusCode.OK) { - echoapp.EchoApp.addRightMessage('Error code: '+status.code+' "'+status.details+'"'); - } - } } ); echoApp.load(); diff --git a/packages/grpc-web/test/generated_code_test.js b/packages/grpc-web/test/generated_code_test.js index ad526891..5943c386 100644 --- a/packages/grpc-web/test/generated_code_test.js +++ b/packages/grpc-web/test/generated_code_test.js @@ -163,11 +163,14 @@ describe('grpc-web generated code (commonjs+grpcwebtext)', function() { // a single data frame with 'aaa' message, encoded 'AAAAAAUKA2FhYQ=='); }; - echoService.echo(request, {'custom-header-1':'value1'}, - function(err, response) { - assert.equal('aaa', response.getMessage()); - done(); - }); + var call = echoService.echo(request, {'custom-header-1':'value1'}, + function(err, response) { + assert.equal('aaa', response.getMessage()); + done(); + }); + call.on('data', (response) => { + assert.fail('should not receive response this way'); + }); }); it('should receive streaming response', function(done) { @@ -229,6 +232,7 @@ describe('grpc-web generated code (commonjs+grpcwebtext)', function() { done(); }); call.on('status', function(status) { + assert.equal(0, status.code); assert.equal('object', typeof status.metadata); assert.equal(false, 'grpc-status' in status.metadata); assert.equal(true, 'x-custom-1' in status.metadata); @@ -259,6 +263,9 @@ describe('grpc-web generated code (commonjs+grpcwebtext)', function() { assert.equal(10, err.code); done(); }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); + }); }); it('should not receive response on non-ok status', function(done) { @@ -295,6 +302,9 @@ describe('grpc-web generated code (commonjs+grpcwebtext)', function() { assert.equal('ababab', status.metadata['x-custom-1']); done(); }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); + }); }); }); @@ -452,7 +462,8 @@ describe('grpc-web generated code: callbacks tests', function() { }); }); - it('should receive error, on html error', function(done) { + it('should receive error, on http error', function(done) { + done = multiDone(done, 2); MockXMLHttpRequest.onSend = function(xhr) { xhr.respond( 400, {'Content-Type': 'application/grpc-web-text'}); @@ -469,7 +480,11 @@ describe('grpc-web generated code: callbacks tests', function() { } ); call.on('status', (status) => { - assert.fail('should not have received a status callback'); + assert.equal(3, status.code); + done(); + }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); }); }); @@ -504,9 +519,13 @@ describe('grpc-web generated code: callbacks tests', function() { assert.equal('ababab', status.metadata['x-custom-1']); done(); }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); + }); }); it('should receive error, on response header error', function(done) { + done = multiDone(done, 2); MockXMLHttpRequest.onSend = function(xhr) { xhr.respond( 200, { @@ -528,7 +547,11 @@ describe('grpc-web generated code: callbacks tests', function() { } ); call.on('status', (status) => { - assert.fail('should not receive a trailing status callback'); + assert.equal(2, status.code); + done(); + }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); }); }); @@ -554,68 +577,49 @@ describe('grpc-web generated code: callbacks tests', function() { } ); call.on('status', (status) => { + assert.equal(0, status.code); // grpc-status should not be part of trailing metadata assert.equal(false, 'grpc-status' in status.metadata); assert.equal(true, 'x-custom-1' in status.metadata); assert.equal('ababab', status.metadata['x-custom-1']); done(); }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); + }); }); - it('should trigger all callbacks', function(done) { - done = multiDone(done, 3); + it('should trigger multiple callbacks on same event', function(done) { + done = multiDone(done, 2); MockXMLHttpRequest.onSend = function(xhr) { xhr.respond( 400, {'Content-Type': 'application/grpc-web-text'}); }; - var call = echoService.echo( - request, {}, - function(err, response) { - if (response) { - assert.fail('should not have received response with non-OK status'); - } else { - assert.equal(3, err.code); // http error 400 mapped to grpc error 3 - } - done(); - } - ); - call.on('status', (status) => { - assert.fail('should not have received a status callback'); + var call = echoService.serverStreamingEcho(request, {}); + + call.on('data', (response) => { + assert.fail('should not have received a data callback'); }); call.on('error', (error) => { - assert.equal(3, error.code); + assert.equal(3, error.code); // http error 400 mapped to grpc error 3 done(); }); call.on('error', (error) => { - assert.equal(3, error.code); + assert.equal(3, error.code); // http error 400 mapped to grpc error 3 done(); }); }); it('should be able to remove callback', function(done) { - done = multiDone(done, 2); MockXMLHttpRequest.onSend = function(xhr) { xhr.respond( 400, {'Content-Type': 'application/grpc-web-text'}); }; - var call = echoService.echo( - request, {}, - function(err, response) { - if (response) { - assert.fail('should not have received response with non-OK status'); - } else { - assert.equal(3, err.code); // http error 400 mapped to grpc error 3 - } - done(); - } - ); - call.on('status', (status) => { - assert.fail('should not have received a status callback'); - }); + var call = echoService.serverStreamingEcho(request, {}); const callbackA = (error) => { - assert.equal(3, error.code); + assert.equal(3, error.code); // http error 400 mapped to grpc error 3 done(); - }; + } const callbackB = (error) => { assert.fail('should not be called'); } @@ -624,6 +628,134 @@ describe('grpc-web generated code: callbacks tests', function() { call.removeListener('error', callbackB); }); + it('should receive initial metadata callback (streaming)', function(done) { + done = multiDone(done, 2); + MockXMLHttpRequest.onSend = function(xhr) { + xhr.respond( + 200, { + 'Content-Type': 'application/grpc-web-text', + 'initial-header-1': 'value1', + }, + // a single data frame with message 'aaa' + 'AAAAAAUKA2FhYQ=='); + }; + var call = echoService.serverStreamingEcho(request, {}); + call.on('data', (response) => { + assert.equal('aaa', response.getMessage()); + done(); + }); + call.on('metadata', (metadata) => { + assert('initial-header-1' in metadata); + assert.equal('value1', metadata['initial-header-1']); + done(); + }); + }); + + it('should receive error, on http error (streaming)', function(done) { + done = multiDone(done, 2); + MockXMLHttpRequest.onSend = function(xhr) { + xhr.respond( + 400, {'Content-Type': 'application/grpc-web-text'}); + }; + var call = echoService.serverStreamingEcho(request, {}); + call.on('data', (response) => { + assert.fail('should not receive data response'); + }); + call.on('status', (status) => { + assert.equal(3, status.code); + done(); + }); + call.on('error', (error) => { + assert.equal(3, error.code); + done(); + }); + }); + + it('should receive error, on grpc error (streaming)', function(done) { + done = multiDone(done, 3); + MockXMLHttpRequest.onSend = function(xhr) { + xhr.respond( + 200, {'Content-Type': 'application/grpc-web-text'}, + // a single data frame with an 'aaa' message, followed by, + // a trailer frame with content 'grpc-status: 2\d\ax-custom-1: ababab' + 'AAAAAAUKA2FhYYAAAAAkZ3JwYy1zdGF0dXM6IDINCngtY3VzdG9tLTE6IGFiYWJhYg0K' + ); + }; + var call = echoService.serverStreamingEcho(request, {}); + call.on('data', (response) => { + // because this is a streaming call, we should still receive data + // callbacks if the error comes in with the trailer frame + assert.equal('aaa', response.getMessage()); + done(); + }); + call.on('error', (error) => { + assert.equal(2, error.code); + assert.equal(true, 'x-custom-1' in error.metadata); + assert.equal('ababab', error.metadata['x-custom-1']); + done(); + }); + call.on('status', (status) => { + assert.equal(2, status.code); + // grpc-status should not be part of trailing metadata + assert.equal(false, 'grpc-status' in status.metadata); + assert.equal(true, 'x-custom-1' in status.metadata); + assert.equal('ababab', status.metadata['x-custom-1']); + done(); + }); + }); + + it('should receive error, on response header error (streaming)', function(done) { + done = multiDone(done, 2); + MockXMLHttpRequest.onSend = function(xhr) { + xhr.respond( + 200, { + 'Content-Type': 'application/grpc-web-text', + 'grpc-status': 2, + 'grpc-message': 'some error', + }); + }; + var call = echoService.serverStreamingEcho(request, {}); + call.on('error', (error) => { + assert.equal(2, error.code); + assert.equal('some error', error.message); + done(); + }); + call.on('status', (status) => { + assert.equal(2, status.code); + assert.equal('some error', status.details); + done(); + }); + }); + + it('should receive status callback (streaming)', function(done) { + done = multiDone(done, 2); + MockXMLHttpRequest.onSend = function(xhr) { + xhr.respond( + 200, {'Content-Type': 'application/grpc-web-text'}, + // a single data frame with an 'aaa' message, followed by, + // a trailer frame with content 'grpc-status: 0\d\ax-custom-1: ababab' + 'AAAAAAUKA2FhYYAAAAAkZ3JwYy1zdGF0dXM6IDANCngtY3VzdG9tLTE6IGFiYWJhYg0K' + ); + }; + var call = echoService.serverStreamingEcho(request, {}); + call.on('data', (response) => { + assert(response); + assert.equal('aaa', response.getMessage()); + done(); + }); + call.on('status', (status) => { + assert.equal(0, status.code); + // grpc-status should not be part of trailing metadata + assert.equal(false, 'grpc-status' in status.metadata); + assert.equal(true, 'x-custom-1' in status.metadata); + assert.equal('ababab', status.metadata['x-custom-1']); + done(); + }); + call.on('error', (error) => { + assert.fail('error callback should not be called for unary calls'); + }); + }); + }); describe('grpc-web generated code (commonjs+dts)', function() {