Change error status when no message is returned in unary calls (#920)

Author: JamesNK

src/Grpc.Net.Client/Internal/GrpcCall.cs

@@ -496,8 +496,12 @@ private async Task RunCall(HttpRequestMessage request, TimeSpan? timeout)
                                 // If it does then throw an error that no message was returned from the server.
                                 GrpcCallLog.MessageNotReturned(Logger);
 
+                                // Change the status code to a more accurate status.
+                                // This is consistent with Grpc.Core client behavior.
+                                status = new Status(StatusCode.Internal, "Failed to deserialize response message.");
+
                                 finished = FinishCall(request, diagnosticSourceEnabled, activity, status.Value);
-                                _responseTcs.TrySetException(new InvalidOperationException("Call did not return a response message."));
+                                SetFailedResult(status.Value);
                             }
 
                             FinishResponseAndCleanUp(status.Value);
@@ -524,19 +528,28 @@ private async Task RunCall(HttpRequestMessage request, TimeSpan? timeout)
                                 singleMessage: true,
                                 _callCts.Token).ConfigureAwait(false);
                             status = GrpcProtocolHelpers.GetResponseStatus(HttpResponse);
-                            FinishResponseAndCleanUp(status.Value);
 
                             if (message == null)
                             {
                                 GrpcCallLog.MessageNotReturned(Logger);
 
+                                if (status.Value.StatusCode == StatusCode.OK)
+                                {
+                                    // Change the status code if OK is returned to a more accurate status.
+                                    // This is consistent with Grpc.Core client behavior.
+                                    status = new Status(StatusCode.Internal, "Failed to deserialize response message.");
+                                }
+
+                                FinishResponseAndCleanUp(status.Value);
                                 finished = FinishCall(request, diagnosticSourceEnabled, activity, status.Value);
+
                                 SetFailedResult(status.Value);
                             }
                             else
                             {
                                 GrpcEventSource.Log.MessageReceived();
 
+                                FinishResponseAndCleanUp(status.Value);
                                 finished = FinishCall(request, diagnosticSourceEnabled, activity, status.Value);
 
                                 if (status.Value.StatusCode == StatusCode.OK)

test/FunctionalTests/Client/UnaryTests.cs

@@ -0,0 +1,68 @@
+#region Copyright notice and license
+
+// Copyright 2019 The gRPC Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#endregion
+
+using System;
+using System.Threading.Tasks;
+using Greet;
+using Grpc.AspNetCore.FunctionalTests.Infrastructure;
+using Grpc.Core;
+using Grpc.Tests.Shared;
+using NUnit.Framework;
+
+namespace Grpc.AspNetCore.FunctionalTests.Client
+{
+    [TestFixture]
+    public class UnaryTests : FunctionalTestBase
+    {
+        [Test]
+        public async Task ThrowErrorWithOK_ClientThrowsFailedToDeserializeError()
+        {
+            SetExpectedErrorsFilter(writeContext =>
+            {
+                if (writeContext.State.ToString() == "Message not returned from unary or client streaming call.")
+                {
+                    return true;
+                }
+
+                return false;
+            });
+
+            Task<HelloReply> UnaryThrowError(HelloRequest request, ServerCallContext context)
+            {
+                return Task.FromException<HelloReply>(new RpcException(new Status(StatusCode.OK, "Message")));
+            }
+
+            // Arrange
+            var method = Fixture.DynamicGrpc.AddUnaryMethod<HelloRequest, HelloReply>(UnaryThrowError);
+            var channel = CreateChannel();
+            var client = TestClientFactory.Create(channel, method);
+
+            // Act
+            var call = client.UnaryCall(new HelloRequest());
+
+            // Assert
+            var ex = await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseAsync).DefaultTimeout();
+
+            Assert.AreEqual(StatusCode.Internal, ex.Status.StatusCode);
+            Assert.AreEqual("Failed to deserialize response message.", ex.Status.Detail);
+
+            Assert.AreEqual(StatusCode.Internal, call.GetStatus().StatusCode);
+            Assert.AreEqual("Failed to deserialize response message.", call.GetStatus().Detail);
+        }
+    }
+}

test/Grpc.Net.Client.Tests/AsyncClientStreamingCallTests.cs

@@ -241,7 +241,8 @@ public async Task ClientStreamWriter_WriteAfterResponseHasFinished_ErrorThrown()
 
             // Assert
             Assert.AreEqual("Can't write the message because the call is complete.", ex.Message);
-            Assert.AreEqual(StatusCode.OK, call.GetStatus().StatusCode);
+            Assert.AreEqual(StatusCode.Internal, call.GetStatus().StatusCode);
+            Assert.AreEqual("Failed to deserialize response message.", call.GetStatus().Detail);
         }
 
         [Test]

test/Grpc.Net.Client.Tests/AsyncUnaryCallTests.cs

@@ -158,14 +158,17 @@ public async Task AsyncUnaryCall_SuccessTrailersOnly_ThrowNoMessageError()
             // Act
             var call = invoker.AsyncUnaryCall<HelloRequest, HelloReply>(ClientTestHelpers.ServiceMethod, string.Empty, new CallOptions(), new HelloRequest());
             var headers = await call.ResponseHeadersAsync.DefaultTimeout();
-            var response = await ExceptionAssert.ThrowsAsync<InvalidOperationException>(() => call.ResponseAsync).DefaultTimeout();
+            var ex = await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseAsync).DefaultTimeout();
 
             // Assert
             Assert.NotNull(responseMessage);
             Assert.IsFalse(responseMessage!.TrailingHeaders.Any()); // sanity check that there are no trailers
 
-            Assert.AreEqual(StatusCode.OK, call.GetStatus().StatusCode);
-            Assert.AreEqual("Detail!", call.GetStatus().Detail);
+            Assert.AreEqual(StatusCode.Internal, ex.Status.StatusCode);
+            Assert.AreEqual("Failed to deserialize response message.", ex.Status.Detail);
+
+            Assert.AreEqual(StatusCode.Internal, call.GetStatus().StatusCode);
+            Assert.AreEqual("Failed to deserialize response message.", call.GetStatus().Detail);
 
             Assert.AreEqual(0, headers.Count);
             Assert.AreEqual(0, call.GetTrailers().Count);