From 13d1abf4447e30b2cef2924656b73044cf40cf8c Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 2 May 2016 09:54:58 -0700 Subject: [PATCH 1/3] improve unary call response handler --- src/csharp/Grpc.Core/Internal/AsyncCall.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs index 016e1b85878..fc3c4a2b2be 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCall.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCall.cs @@ -407,12 +407,15 @@ namespace Grpc.Core.Internal /// /// Handler for unary response completion. /// - private void HandleUnaryResponse(bool success, ClientSideStatus receivedStatus, byte[] receivedMessage, Metadata responseHeaders) + private void HandleUnaryResponse(bool sucess, ClientSideStatus receivedStatus, byte[] receivedMessage, Metadata responseHeaders) { + // NOTE: because this event is a result of batch containing GRPC_OP_RECV_STATUS_ON_CLIENT, + // success will be always set to true. + using (Profilers.ForCurrentThread().NewScope("AsyncCall.HandleUnaryResponse")) { TResponse msg = default(TResponse); - var deserializeException = success ? TryDeserialize(receivedMessage, out msg) : null; + var deserializeException = TryDeserialize(receivedMessage, out msg); lock (myLock) { @@ -425,14 +428,13 @@ namespace Grpc.Core.Internal finishedStatus = receivedStatus; ReleaseResourcesIfPossible(); - } responseHeadersTcs.SetResult(responseHeaders); var status = receivedStatus.Status; - if (!success || status.StatusCode != StatusCode.OK) + if (status.StatusCode != StatusCode.OK) { unaryResponseTcs.SetException(new RpcException(status)); return; @@ -447,6 +449,9 @@ namespace Grpc.Core.Internal /// private void HandleFinished(bool success, ClientSideStatus receivedStatus) { + // NOTE: because this event is a result of batch containing GRPC_OP_RECV_STATUS_ON_CLIENT, + // success will be always set to true. + lock (myLock) { finished = true; @@ -457,7 +462,7 @@ namespace Grpc.Core.Internal var status = receivedStatus.Status; - if (!success || status.StatusCode != StatusCode.OK) + if (status.StatusCode != StatusCode.OK) { streamingCallFinishedTcs.SetException(new RpcException(status)); return; From a4a627030eb34e8e2f068f1ab327810cd4c3354c Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 2 May 2016 09:55:18 -0700 Subject: [PATCH 2/3] add more tests --- .../Grpc.Core.Tests/Internal/AsyncCallTest.cs | 109 ++++++++++++++++-- 1 file changed, 98 insertions(+), 11 deletions(-) diff --git a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs index 60530d32508..a678e4dafe1 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs @@ -64,28 +64,115 @@ namespace Grpc.Core.Internal.Tests } [Test] - public void AsyncUnary_CompletionSuccess() + public void AsyncUnary_CanBeStartedOnlyOnce() + { + asyncCall.UnaryCallAsync("request1"); + Assert.Throws(typeof(InvalidOperationException), + () => asyncCall.UnaryCallAsync("abc")); + } + + [Test] + public void AsyncUnary_StreamingOperationsNotAllowed() + { + asyncCall.UnaryCallAsync("request1"); + Assert.Throws(typeof(InvalidOperationException), + () => asyncCall.StartReadMessage((x,y) => {})); + Assert.Throws(typeof(InvalidOperationException), + () => asyncCall.StartSendMessage("abc", new WriteFlags(), (x,y) => {})); + } + + [Test] + public void AsyncUnary_Success() + { + var resultTask = asyncCall.UnaryCallAsync("request1"); + fakeCall.UnaryResponseClientHandler(true, + new ClientSideStatus(Status.DefaultSuccess, new Metadata()), + CreateResponsePayload(), + new Metadata()); + + AssertUnaryResponseSuccess(asyncCall, fakeCall, resultTask); + } + + [Test] + public void AsyncUnary_NonSuccessStatusCode() + { + var resultTask = asyncCall.UnaryCallAsync("request1"); + fakeCall.UnaryResponseClientHandler(true, + CreateClientSideStatus(StatusCode.InvalidArgument), + CreateResponsePayload(), + new Metadata()); + + AssertUnaryResponseError(asyncCall, fakeCall, resultTask, StatusCode.InvalidArgument); + } + + [Test] + public void AsyncUnary_NullResponsePayload() + { + var resultTask = asyncCall.UnaryCallAsync("request1"); + fakeCall.UnaryResponseClientHandler(true, + new ClientSideStatus(Status.DefaultSuccess, new Metadata()), + null, + new Metadata()); + + // failure to deserialize will result in InvalidArgument status. + AssertUnaryResponseError(asyncCall, fakeCall, resultTask, StatusCode.Internal); + } + + [Test] + public void ClientStreaming_NoRequest_Success() + { + var resultTask = asyncCall.ClientStreamingCallAsync(); + fakeCall.UnaryResponseClientHandler(true, + new ClientSideStatus(Status.DefaultSuccess, new Metadata()), + CreateResponsePayload(), + new Metadata()); + + AssertUnaryResponseSuccess(asyncCall, fakeCall, resultTask); + } + + [Test] + public void ClientStreaming_NoRequest_NonSuccessStatusCode() + { + var resultTask = asyncCall.ClientStreamingCallAsync(); + fakeCall.UnaryResponseClientHandler(true, + CreateClientSideStatus(StatusCode.InvalidArgument), + CreateResponsePayload(), + new Metadata()); + + AssertUnaryResponseError(asyncCall, fakeCall, resultTask, StatusCode.InvalidArgument); + } + + ClientSideStatus CreateClientSideStatus(StatusCode statusCode) + { + return new ClientSideStatus(new Status(statusCode, ""), new Metadata()); + } + + byte[] CreateResponsePayload() + { + return Marshallers.StringMarshaller.Serializer("response1"); + } + + static void AssertUnaryResponseSuccess(AsyncCall asyncCall, FakeNativeCall fakeCall, Task resultTask) { - var resultTask = asyncCall.UnaryCallAsync("abc"); - fakeCall.UnaryResponseClientHandler(true, new ClientSideStatus(Status.DefaultSuccess, new Metadata()), new byte[] { 1, 2, 3 }, new Metadata()); Assert.IsTrue(resultTask.IsCompleted); Assert.IsTrue(fakeCall.IsDisposed); + Assert.AreEqual(Status.DefaultSuccess, asyncCall.GetStatus()); + Assert.AreEqual(0, asyncCall.ResponseHeadersAsync.Result.Count); + Assert.AreEqual(0, asyncCall.GetTrailers().Count); + Assert.AreEqual("response1", resultTask.Result); } - [Test] - public void AsyncUnary_CompletionFailure() + static void AssertUnaryResponseError(AsyncCall asyncCall, FakeNativeCall fakeCall, Task resultTask, StatusCode expectedStatusCode) { - var resultTask = asyncCall.UnaryCallAsync("abc"); - fakeCall.UnaryResponseClientHandler(false, new ClientSideStatus(new Status(StatusCode.Internal, ""), null), new byte[] { 1, 2, 3 }, new Metadata()); - Assert.IsTrue(resultTask.IsCompleted); Assert.IsTrue(fakeCall.IsDisposed); - Assert.AreEqual(StatusCode.Internal, asyncCall.GetStatus().StatusCode); - Assert.IsNull(asyncCall.GetTrailers()); + Assert.AreEqual(expectedStatusCode, asyncCall.GetStatus().StatusCode); var ex = Assert.ThrowsAsync(async () => await resultTask); - Assert.AreEqual(StatusCode.Internal, ex.Status.StatusCode); + Assert.AreEqual(expectedStatusCode, ex.Status.StatusCode); + Assert.AreEqual(0, asyncCall.ResponseHeadersAsync.Result.Count); + Assert.AreEqual(0, asyncCall.GetTrailers().Count); } internal class FakeNativeCall : INativeCall From 08e1f755a3b456ce2e86db539068278407317a3c Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 2 May 2016 12:07:21 -0700 Subject: [PATCH 3/3] fix typo --- src/csharp/Grpc.Core/Internal/AsyncCall.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs index fc3c4a2b2be..50ba617cdb1 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCall.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCall.cs @@ -407,7 +407,7 @@ namespace Grpc.Core.Internal /// /// Handler for unary response completion. /// - private void HandleUnaryResponse(bool sucess, ClientSideStatus receivedStatus, byte[] receivedMessage, Metadata responseHeaders) + private void HandleUnaryResponse(bool success, ClientSideStatus receivedStatus, byte[] receivedMessage, Metadata responseHeaders) { // NOTE: because this event is a result of batch containing GRPC_OP_RECV_STATUS_ON_CLIENT, // success will be always set to true.