From a610e32e4b07d860048d478e2f2c851c7c9bb4d6 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 13 Sep 2016 16:53:05 +0200 Subject: throw correct exception failed writes --- .../Internal/AsyncCallServerTest.cs | 4 +- .../Grpc.Core.Tests/Internal/AsyncCallTest.cs | 74 +++++++++++++++++++++- src/csharp/Grpc.Core/Internal/AsyncCall.cs | 22 ++++++- src/csharp/Grpc.Core/Internal/AsyncCallBase.cs | 32 +++++++++- src/csharp/Grpc.Core/Internal/AsyncCallServer.cs | 6 ++ 5 files changed, 129 insertions(+), 9 deletions(-) (limited to 'src/csharp') diff --git a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallServerTest.cs b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallServerTest.cs index c35aaf680f..09790120d1 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallServerTest.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallServerTest.cs @@ -33,6 +33,7 @@ using System; using System.Collections.Generic; +using System.IO; using System.Runtime.InteropServices; using System.Threading.Tasks; @@ -149,8 +150,7 @@ namespace Grpc.Core.Internal.Tests var writeTask = responseStream.WriteAsync("request1"); fakeCall.SendCompletionHandler(false); - // TODO(jtattermusch): should we throw a different exception type instead? - Assert.ThrowsAsync(typeof(InvalidOperationException), async () => await writeTask); + Assert.ThrowsAsync(typeof(IOException), async () => await writeTask); fakeCall.ReceivedCloseOnServerHandler(true, cancelled: true); AssertFinished(asyncCallServer, fakeCall, finishedTask); diff --git a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs index 98e27a17a1..8ee4d184ab 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/AsyncCallTest.cs @@ -180,21 +180,46 @@ namespace Grpc.Core.Internal.Tests } [Test] - public void ClientStreaming_WriteCompletionFailure() + public void ClientStreaming_WriteFailureThrowsRpcException() { var resultTask = asyncCall.ClientStreamingCallAsync(); var requestStream = new ClientRequestStream(asyncCall); var writeTask = requestStream.WriteAsync("request1"); fakeCall.SendCompletionHandler(false); - // TODO: maybe IOException or waiting for RPCException is more appropriate here. - Assert.ThrowsAsync(typeof(InvalidOperationException), async () => await writeTask); + + // The write will wait for call to finish to receive the status code. + Assert.IsFalse(writeTask.IsCompleted); + + fakeCall.UnaryResponseClientHandler(true, + CreateClientSideStatus(StatusCode.Internal), + null, + new Metadata()); + + var ex = Assert.ThrowsAsync(async () => await writeTask); + Assert.AreEqual(StatusCode.Internal, ex.Status.StatusCode); + + AssertUnaryResponseError(asyncCall, fakeCall, resultTask, StatusCode.Internal); + } + + [Test] + public void ClientStreaming_WriteFailureThrowsRpcException2() + { + var resultTask = asyncCall.ClientStreamingCallAsync(); + var requestStream = new ClientRequestStream(asyncCall); + + var writeTask = requestStream.WriteAsync("request1"); fakeCall.UnaryResponseClientHandler(true, CreateClientSideStatus(StatusCode.Internal), null, new Metadata()); + fakeCall.SendCompletionHandler(false); + + var ex = Assert.ThrowsAsync(async () => await writeTask); + Assert.AreEqual(StatusCode.Internal, ex.Status.StatusCode); + AssertUnaryResponseError(asyncCall, fakeCall, resultTask, StatusCode.Internal); } @@ -415,6 +440,49 @@ namespace Grpc.Core.Internal.Tests Assert.DoesNotThrowAsync(async () => await requestStream.CompleteAsync()); } + [Test] + public void DuplexStreaming_WriteFailureThrowsRpcException() + { + asyncCall.StartDuplexStreamingCall(); + var requestStream = new ClientRequestStream(asyncCall); + var responseStream = new ClientResponseStream(asyncCall); + + var writeTask = requestStream.WriteAsync("request1"); + fakeCall.SendCompletionHandler(false); + + // The write will wait for call to finish to receive the status code. + Assert.IsFalse(writeTask.IsCompleted); + + var readTask = responseStream.MoveNext(); + fakeCall.ReceivedMessageHandler(true, null); + fakeCall.ReceivedStatusOnClientHandler(true, CreateClientSideStatus(StatusCode.PermissionDenied)); + + var ex = Assert.ThrowsAsync(async () => await writeTask); + Assert.AreEqual(StatusCode.PermissionDenied, ex.Status.StatusCode); + + AssertStreamingResponseError(asyncCall, fakeCall, readTask, StatusCode.PermissionDenied); + } + + [Test] + public void DuplexStreaming_WriteFailureThrowsRpcException2() + { + asyncCall.StartDuplexStreamingCall(); + var requestStream = new ClientRequestStream(asyncCall); + var responseStream = new ClientResponseStream(asyncCall); + + var writeTask = requestStream.WriteAsync("request1"); + + var readTask = responseStream.MoveNext(); + fakeCall.ReceivedMessageHandler(true, null); + fakeCall.ReceivedStatusOnClientHandler(true, CreateClientSideStatus(StatusCode.PermissionDenied)); + fakeCall.SendCompletionHandler(false); + + var ex = Assert.ThrowsAsync(async () => await writeTask); + Assert.AreEqual(StatusCode.PermissionDenied, ex.Status.StatusCode); + + AssertStreamingResponseError(asyncCall, fakeCall, readTask, StatusCode.PermissionDenied); + } + [Test] public void DuplexStreaming_WriteAfterCancellationRequestThrowsTaskCanceledException() { diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs index f549c52876..db40b553ce 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCall.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCall.cs @@ -341,6 +341,11 @@ namespace Grpc.Core.Internal get { return true; } } + protected override Exception GetRpcExceptionClientOnly() + { + return new RpcException(finishedStatus.Value.Status); + } + protected override Task CheckSendAllowedOrEarlyResult() { var earlyResult = CheckSendPreconditionsClientSide(); @@ -452,6 +457,7 @@ namespace Grpc.Core.Internal using (Profilers.ForCurrentThread().NewScope("AsyncCall.HandleUnaryResponse")) { + TaskCompletionSource delayedTcs; TResponse msg = default(TResponse); var deserializeException = TryDeserialize(receivedMessage, out msg); @@ -464,14 +470,19 @@ namespace Grpc.Core.Internal receivedStatus = new ClientSideStatus(DeserializeResponseFailureStatus, receivedStatus.Trailers); } finishedStatus = receivedStatus; + delayedTcs = delayedStreamingWriteTcs; ReleaseResourcesIfPossible(); } responseHeadersTcs.SetResult(responseHeaders); - var status = receivedStatus.Status; + if (delayedTcs != null) + { + delayedTcs.SetException(GetRpcExceptionClientOnly()); + } + var status = receivedStatus.Status; if (status.StatusCode != StatusCode.OK) { unaryResponseTcs.SetException(new RpcException(status)); @@ -490,16 +501,23 @@ namespace Grpc.Core.Internal // NOTE: because this event is a result of batch containing GRPC_OP_RECV_STATUS_ON_CLIENT, // success will be always set to true. + TaskCompletionSource delayedTcs; + lock (myLock) { finished = true; finishedStatus = receivedStatus; + delayedTcs = delayedStreamingWriteTcs; ReleaseResourcesIfPossible(); } - var status = receivedStatus.Status; + if (delayedTcs != null) + { + delayedTcs.SetException(GetRpcExceptionClientOnly()); + } + var status = receivedStatus.Status; if (status.StatusCode != StatusCode.OK) { streamingCallFinishedTcs.SetException(new RpcException(status)); diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs index eb9c3ea62d..b27baba942 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs @@ -68,6 +68,7 @@ namespace Grpc.Core.Internal protected TaskCompletionSource streamingReadTcs; // Completion of a pending streaming read if not null. protected TaskCompletionSource streamingWriteTcs; // Completion of a pending streaming write or send close from client if not null. + protected TaskCompletionSource delayedStreamingWriteTcs; protected TaskCompletionSource sendStatusFromServerTcs; protected bool readingDone; // True if last read (i.e. read with null payload) was already received. @@ -200,6 +201,12 @@ namespace Grpc.Core.Internal get; } + /// + /// Returns an exception to throw for a failed send operation. + /// It is only allowed to call this method for a call that has already finished. + /// + protected abstract Exception GetRpcExceptionClientOnly(); + private void ReleaseResources() { if (call != null) @@ -252,18 +259,39 @@ namespace Grpc.Core.Internal /// protected void HandleSendFinished(bool success) { + bool delayCompletion = false; TaskCompletionSource origTcs = null; lock (myLock) { origTcs = streamingWriteTcs; streamingWriteTcs = null; + if (!success && !finished && IsClient) + { + // We should be setting this only once per call, following writes will be short circuited. + GrpcPreconditions.CheckState (delayedStreamingWriteTcs == null); + delayedStreamingWriteTcs = origTcs; + delayCompletion = true; + } + ReleaseResourcesIfPossible(); } if (!success) { - origTcs.SetException(new InvalidOperationException("Send failed")); + if (!delayCompletion) + { + if (IsClient) + { + GrpcPreconditions.CheckState(finished); // implied by !success && !delayCompletion && IsClient + origTcs.SetException(GetRpcExceptionClientOnly()); + } + else + { + origTcs.SetException (new IOException("Error sending from server.")); + } + } + // if delayCompletion == true, postpone SetException until call finishes. } else { @@ -283,7 +311,7 @@ namespace Grpc.Core.Internal if (!success) { - sendStatusFromServerTcs.SetException(new InvalidOperationException("Error sending status from server.")); + sendStatusFromServerTcs.SetException(new IOException("Error sending status from server.")); } else { diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs index 56c23ba3ef..50fdfa9006 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs @@ -33,6 +33,7 @@ using System; using System.Diagnostics; +using System.IO; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -193,6 +194,11 @@ namespace Grpc.Core.Internal get { return false; } } + protected override Exception GetRpcExceptionClientOnly() + { + throw new InvalidOperationException("Call be only called for client calls"); + } + protected override void OnAfterReleaseResources() { server.RemoveCallReference(this); -- cgit v1.2.3