From a774583de01870a0590faa4041bdb030b1cbe31c Mon Sep 17 00:00:00 2001 From: Nic Hite Date: Tue, 29 Sep 2020 01:01:18 -0700 Subject: [PATCH] Beef up exception handling in gRPC code. (#360) * Beef up exception handling in gRPC code. * Verify default stacktrace isn't used in exceptions --- CHANGELOG.md | 5 ++++ lib/src/client/call.dart | 22 ++++++++------ lib/src/client/transport/transport.dart | 2 +- lib/src/client/transport/xhr_transport.dart | 22 +++++++++----- pubspec.yaml | 2 +- test/client_tests/client_test.dart | 21 +++++++++++++ .../client_xhr_transport_test.dart | 24 +++++++-------- test/src/client_utils.dart | 30 ++++++++++++------- 8 files changed, 86 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index acd5ac9..3454733 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.4.1 + +* Plumb stacktraces through request / response stream error handlers. +* Catch and forward any errors decoding the response. + ## 2.4.0 * Add the ability to bypass CORS preflight requests. diff --git a/lib/src/client/call.dart b/lib/src/client/call.dart index c93da8c..61f2583 100644 --- a/lib/src/client/call.dart +++ b/lib/src/client/call.dart @@ -266,8 +266,8 @@ class ClientCall implements Response { } /// Emit an error response to the user, and tear down this call. - void _responseError(GrpcError error) { - _responses.addError(error); + void _responseError(GrpcError error, [StackTrace stackTrace]) { + _responses.addError(error, stackTrace); _timeoutTimer?.cancel(); _requestSubscription?.cancel(); _responseSubscription.cancel(); @@ -287,8 +287,12 @@ class ClientCall implements Response { _responseError(GrpcError.unimplemented('Received data after trailers')); return; } - _responses.add(_method.responseDeserializer(data.data)); - _hasReceivedResponses = true; + try { + _responses.add(_method.responseDeserializer(data.data)); + _hasReceivedResponses = true; + } catch (e, s) { + _responseError(GrpcError.dataLoss('Error parsing response'), s); + } } else if (data is GrpcMetadata) { if (!_headers.isCompleted) { // TODO(jakobr): Parse, and extract common headers. @@ -319,12 +323,12 @@ class ClientCall implements Response { /// Handler for response errors. Forward the error to the [_responses] stream, /// wrapped if necessary. - void _onResponseError(error) { + void _onResponseError(error, StackTrace stackTrace) { if (error is GrpcError) { - _responseError(error); + _responseError(error, stackTrace); return; } - _responseError(GrpcError.unknown(error.toString())); + _responseError(GrpcError.unknown(error.toString()), stackTrace); } /// Handles closure of the response stream. Verifies that server has sent @@ -362,12 +366,12 @@ class ClientCall implements Response { /// Error handler for the requests stream. Something went wrong while trying /// to send the request to the server. Abort the request, and forward the /// error to the user code on the [_responses] stream. - void _onRequestError(error, [StackTrace stackTrace]) { + void _onRequestError(error, StackTrace stackTrace) { if (error is! GrpcError) { error = GrpcError.unknown(error.toString()); } - _responses.addError(error); + _responses.addError(error, stackTrace); _timeoutTimer?.cancel(); _responses.close(); _requestSubscription?.cancel(); diff --git a/lib/src/client/transport/transport.dart b/lib/src/client/transport/transport.dart index dff90b2..e14c4ca 100644 --- a/lib/src/client/transport/transport.dart +++ b/lib/src/client/transport/transport.dart @@ -19,7 +19,7 @@ import '../../shared/message.dart'; typedef void SocketClosedHandler(); typedef void ActiveStateHandler(bool isActive); -typedef void ErrorHandler(error); +typedef void ErrorHandler(error, StackTrace stackTrace); abstract class GrpcTransportStream { Stream get incomingMessages; diff --git a/lib/src/client/transport/xhr_transport.dart b/lib/src/client/transport/xhr_transport.dart index 4396f49..30ceabb 100644 --- a/lib/src/client/transport/xhr_transport.dart +++ b/lib/src/client/transport/xhr_transport.dart @@ -68,8 +68,10 @@ class XhrTransportStream implements GrpcTransportStream { break; case HttpRequest.DONE: if (_request.status != 200) { - _onError(GrpcError.unavailable( - 'XhrConnection status ${_request.status}')); + _onError( + GrpcError.unavailable( + 'XhrConnection status ${_request.status}'), + StackTrace.current); } else { _close(); } @@ -81,7 +83,8 @@ class XhrTransportStream implements GrpcTransportStream { if (_incomingMessages.isClosed) { return; } - _onError(GrpcError.unavailable('XhrConnection connection-error')); + _onError(GrpcError.unavailable('XhrConnection connection-error'), + StackTrace.current); terminate(); }); @@ -113,21 +116,24 @@ class XhrTransportStream implements GrpcTransportStream { _onHeadersReceived() { final contentType = _request.getResponseHeader(_contentTypeKey); if (_request.status != 200) { - _onError( - GrpcError.unavailable('XhrConnection status ${_request.status}')); + _onError(GrpcError.unavailable('XhrConnection status ${_request.status}'), + StackTrace.current); return; } if (contentType == null) { - _onError(GrpcError.unavailable('XhrConnection missing Content-Type')); + _onError(GrpcError.unavailable('XhrConnection missing Content-Type'), + StackTrace.current); return; } if (!_checkContentType(contentType)) { _onError( - GrpcError.unavailable('XhrConnection bad Content-Type $contentType')); + GrpcError.unavailable('XhrConnection bad Content-Type $contentType'), + StackTrace.current); return; } if (_request.response == null) { - _onError(GrpcError.unavailable('XhrConnection request null response')); + _onError(GrpcError.unavailable('XhrConnection request null response'), + StackTrace.current); return; } diff --git a/pubspec.yaml b/pubspec.yaml index e4a2df1..15192be 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,7 +1,7 @@ name: grpc description: Dart implementation of gRPC, a high performance, open-source universal RPC framework. -version: 2.4.0 +version: 2.4.1 author: Dart Team homepage: https://github.com/dart-lang/grpc-dart diff --git a/test/client_tests/client_test.dart b/test/client_tests/client_test.dart index 70480df..ca22de7 100644 --- a/test/client_tests/client_test.dart +++ b/test/client_tests/client_test.dart @@ -290,6 +290,27 @@ void main() { ); }); + test('Call throws if unable to decode response', () async { + const responseValue = 19; + + void handleRequest(StreamMessage message) { + harness + ..sendResponseHeader() + ..sendResponseValue(responseValue) + ..sendResponseTrailer(); + } + + harness.client = TestClient(harness.channel, decode: (bytes) { + throw "error decoding"; + }); + + await harness.runFailureTest( + clientCall: harness.client.unary(dummyValue), + expectedException: GrpcError.dataLoss('Error parsing response'), + serverHandlers: [handleRequest], + ); + }); + test('Call forwards known response stream errors', () async { final expectedException = GrpcError.dataLoss('Oops!'); diff --git a/test/client_tests/client_xhr_transport_test.dart b/test/client_tests/client_xhr_transport_test.dart index 67887d2..26f02f1 100644 --- a/test/client_tests/client_xhr_transport_test.dart +++ b/test/client_tests/client_xhr_transport_test.dart @@ -69,7 +69,7 @@ void main() { final connection = MockXhrClientConnection(); connection.makeRequest('path', Duration(seconds: 10), metadata, - (error) => fail(error.toString())); + (error, _) => fail(error.toString())); verify(connection.latestRequest .setRequestHeader('Content-Type', 'application/grpc-web+proto')); @@ -88,7 +88,7 @@ void main() { final connection = MockXhrClientConnection(); connection.makeRequest('path', Duration(seconds: 10), metadata, - (error) => fail(error.toString()), + (error, _) => fail(error.toString()), callOptions: WebCallOptions(bypassCorsPreflight: true)); expect(metadata, isEmpty); @@ -110,7 +110,7 @@ void main() { final connection = MockXhrClientConnection(); connection.makeRequest('/path', Duration(seconds: 10), metadata, - (error) => fail(error.toString())); + (error, _) => fail(error.toString())); expect(metadata, { 'header_1': 'value_1', @@ -127,7 +127,7 @@ void main() { final connection = MockXhrClientConnection(); connection.makeRequest('/path', Duration(seconds: 10), metadata, - (error) => fail(error.toString())); + (error, _) => fail(error.toString())); expect(metadata, { 'header_1': 'value_1', 'CONTENT-TYPE': 'application/json+protobuf', @@ -138,7 +138,7 @@ void main() { 'content-type': 'application/json+protobuf' }; connection.makeRequest('/path', Duration(seconds: 10), lowerMetadata, - (error) => fail(error.toString())); + (error, _) => fail(error.toString())); expect(lowerMetadata, { 'header_1': 'value_1', 'content-type': 'application/json+protobuf', @@ -151,7 +151,7 @@ void main() { final connection = MockXhrClientConnection(); connection.makeRequest('path', Duration(seconds: 10), metadata, - (error) => fail(error.toString()), + (error, _) => fail(error.toString()), callOptions: WebCallOptions(withCredentials: true)); expect(metadata, { @@ -182,7 +182,7 @@ void main() { final connection = MockXhrClientConnection(); final stream = connection.makeRequest('path', Duration(seconds: 10), - metadata, (error) => fail(error.toString())); + metadata, (error, _) => fail(error.toString())); final data = List.filled(10, 0); stream.outgoingMessages.add(data); @@ -202,7 +202,7 @@ void main() { final transport = MockXhrClientConnection(); final stream = transport.makeRequest('test_path', Duration(seconds: 10), - metadata, (error) => fail(error.toString())); + metadata, (error, _) => fail(error.toString())); stream.incomingMessages.listen((message) { expect(message, TypeMatcher()); @@ -223,7 +223,7 @@ void main() { final connection = MockXhrClientConnection(); final stream = connection.makeRequest('test_path', Duration(seconds: 10), - {}, (error) => fail(error.toString())); + {}, (error, _) => fail(error.toString())); final encodedTrailers = frame(trailers.entries .map((e) => '${e.key}:${e.value}') @@ -254,7 +254,7 @@ void main() { final connection = MockXhrClientConnection(); final stream = connection.makeRequest('test_path', Duration(seconds: 10), - {}, (error) => fail(error.toString())); + {}, (error, _) => fail(error.toString())); final encoded = frame(''.codeUnits); encoded[0] = 0x80; // Mark this frame as trailers. @@ -285,7 +285,7 @@ void main() { final connection = MockXhrClientConnection(); final stream = connection.makeRequest('test_path', Duration(seconds: 10), - metadata, (error) => fail(error.toString())); + metadata, (error, _) => fail(error.toString())); final data = List.filled(10, 224); final encoded = frame(data); final encodedString = String.fromCharCodes(encoded); @@ -315,7 +315,7 @@ void main() { final connection = MockXhrClientConnection(); final stream = connection.makeRequest('test_path', Duration(seconds: 10), - metadata, (error) => fail(error.toString())); + metadata, (error, _) => fail(error.toString())); final data = >[ List.filled(10, 224), diff --git a/test/src/client_utils.dart b/test/src/client_utils.dart index 1dd629b..c5a6c20 100644 --- a/test/src/client_utils.dart +++ b/test/src/client_utils.dart @@ -69,17 +69,24 @@ class FakeChannel extends ClientChannel { typedef ServerMessageHandler = void Function(StreamMessage message); class TestClient extends Client { - static final _$unary = - ClientMethod('/Test/Unary', mockEncode, mockDecode); - static final _$clientStreaming = - ClientMethod('/Test/ClientStreaming', mockEncode, mockDecode); - static final _$serverStreaming = - ClientMethod('/Test/ServerStreaming', mockEncode, mockDecode); - static final _$bidirectional = - ClientMethod('/Test/Bidirectional', mockEncode, mockDecode); + ClientMethod _$unary; + ClientMethod _$clientStreaming; + ClientMethod _$serverStreaming; + ClientMethod _$bidirectional; - TestClient(ClientChannel channel, {CallOptions options}) - : super(channel, options: options); + final int Function(List value) decode; + + TestClient(ClientChannel channel, + {CallOptions options, this.decode: mockDecode}) + : super(channel, options: options) { + _$unary = ClientMethod('/Test/Unary', mockEncode, decode); + _$clientStreaming = + ClientMethod('/Test/ClientStreaming', mockEncode, decode); + _$serverStreaming = + ClientMethod('/Test/ServerStreaming', mockEncode, decode); + _$bidirectional = + ClientMethod('/Test/Bidirectional', mockEncode, decode); + } ResponseFuture unary(int request, {CallOptions options}) { final call = @@ -201,8 +208,9 @@ class ClientHarness { try { await future; fail('Did not throw'); - } catch (e) { + } catch (e, st) { expect(e, exception); + expect(st, isNot(equals(StackTrace.current))); } }