From 135f433bcd212fc251eb70b3ec41cd28515dc65e Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Sun, 27 Oct 2024 21:29:45 -0700
Subject: [PATCH] Revert "stub: Ignore unary response on server if status is
 not OK" (#11636) (#11640)

This reverts commit 99f86835ed8a1c960d532e58b5fd51ec1bb99825.

The change doesn't handle `null` messages, which don't happen with
protobuf, but can happen with other marshallers, especially in tests.
See cl/689445172

This will reopen #5969.
---
 .../main/java/io/grpc/stub/ServerCalls.java   | 24 ++-------
 .../java/io/grpc/stub/ServerCallsTest.java    | 54 -------------------
 2 files changed, 4 insertions(+), 74 deletions(-)

diff --git a/stub/src/main/java/io/grpc/stub/ServerCalls.java b/stub/src/main/java/io/grpc/stub/ServerCalls.java
index 6c44455153..7990a5b34c 100644
--- a/stub/src/main/java/io/grpc/stub/ServerCalls.java
+++ b/stub/src/main/java/io/grpc/stub/ServerCalls.java
@@ -335,7 +335,6 @@ public final class ServerCalls {
     private boolean aborted = false;
     private boolean completed = false;
     private Runnable onCloseHandler;
-    private RespT unaryResponse;
 
     // Non private to avoid synthetic class
     ServerCallStreamObserverImpl(ServerCall<ReqT, RespT> call, boolean serverStreamingOrBidi) {
@@ -374,22 +373,15 @@ public final class ServerCalls {
       }
       checkState(!aborted, "Stream was terminated by error, no further calls are allowed");
       checkState(!completed, "Stream is already completed, no further calls are allowed");
-      if (serverStreamingOrBidi) {
-        if (!sentHeaders) {
-          call.sendHeaders(new Metadata());
-          sentHeaders = true;
-        }
-        call.sendMessage(response);
-      } else {
-        unaryResponse = response;
+      if (!sentHeaders) {
+        call.sendHeaders(new Metadata());
+        sentHeaders = true;
       }
+      call.sendMessage(response);
     }
 
     @Override
     public void onError(Throwable t) {
-      if (!serverStreamingOrBidi) {
-        unaryResponse = null;
-      }
       Metadata metadata = Status.trailersFromThrowable(t);
       if (metadata == null) {
         metadata = new Metadata();
@@ -400,14 +392,6 @@ public final class ServerCalls {
 
     @Override
     public void onCompleted() {
-      if (!serverStreamingOrBidi && unaryResponse != null) {
-        if (!sentHeaders) {
-          call.sendHeaders(new Metadata());
-          sentHeaders = true;
-        }
-        call.sendMessage(unaryResponse);
-        unaryResponse = null;
-      }
       call.close(Status.OK, new Metadata());
       completed = true;
     }
diff --git a/stub/src/test/java/io/grpc/stub/ServerCallsTest.java b/stub/src/test/java/io/grpc/stub/ServerCallsTest.java
index 8d65be19e2..1e51ac1011 100644
--- a/stub/src/test/java/io/grpc/stub/ServerCallsTest.java
+++ b/stub/src/test/java/io/grpc/stub/ServerCallsTest.java
@@ -33,7 +33,6 @@ import io.grpc.ServerCallHandler;
 import io.grpc.ServerServiceDefinition;
 import io.grpc.ServiceDescriptor;
 import io.grpc.Status;
-import io.grpc.Status.Code;
 import io.grpc.StatusRuntimeException;
 import io.grpc.inprocess.InProcessChannelBuilder;
 import io.grpc.inprocess.InProcessServerBuilder;
@@ -621,59 +620,6 @@ public class ServerCallsTest {
     assertArrayEquals(new int[]{0, 1, 1, 2, 2, 2}, receivedMessages);
   }
 
-  @Test
-  public void serverUnaryResponseMsgWithOkStatus() {
-    ServerCallHandler<Integer, Integer> serverCallHandler =
-        ServerCalls.asyncUnaryCall(
-            new ServerCalls.UnaryMethod<Integer, Integer>() {
-              @Override
-              public void invoke(Integer request, StreamObserver<Integer> responseObserver) {
-                responseObserver.onNext(request);
-                responseObserver.onCompleted();
-              }
-            });
-    ServerCall.Listener<Integer> callListener =
-        serverCallHandler.startCall(serverCall, new Metadata());
-    serverCall.isReady = true;
-    serverCall.isCancelled = false;
-    callListener.onReady();
-    callListener.onMessage(1);
-    callListener.onHalfClose();
-
-    assertThat(serverCall.status.getCode()).isEqualTo(Code.OK);
-    assertThat(serverCall.responses).containsExactly(1);
-  }
-
-  @Test
-  public void serverUnaryResponseMsgWithNotOkStatus() {
-    ServerCallHandler<Integer, Integer> serverCallHandler =
-        ServerCalls.asyncUnaryCall(
-            new ServerCalls.UnaryMethod<Integer, Integer>() {
-              @Override
-              public void invoke(Integer request, StreamObserver<Integer> responseObserver) {
-                responseObserver.onNext(request);
-                responseObserver.onError(
-                    Status.INTERNAL
-                        .withDescription("Response message is null for unary call")
-                        .asRuntimeException());
-              }
-            });
-
-    ServerCall.Listener<Integer> callListener =
-        serverCallHandler.startCall(serverCall, new Metadata());
-
-    serverCall.isReady = true;
-    serverCall.isCancelled = false;
-    callListener.onReady();
-    callListener.onMessage(1);
-    callListener.onHalfClose();
-
-    assertThat(serverCall.status.getCode()).isEqualTo(Code.INTERNAL);
-    assertThat(serverCall.status.getDescription())
-        .isEqualTo("Response message is null for unary call");
-    assertThat(serverCall.responses).isEmpty();
-  }
-
   public static class IntegerMarshaller implements MethodDescriptor.Marshaller<Integer> {
     @Override
     public InputStream stream(Integer value) {