core: Don't mark calls as cancelled if they are successfully completed. (grpc#8408)

temawi · web-flow · commit e45aab085c45 · 2021-08-20T14:42:01.000-07:00
The semantics around cancel vary slightly between ServerCall and CancellableContext - the context should always be cancelled regardless of the outcome of the call while the ServerCall should only be cancelled on a non-OK status. This fixes a bug where the ServerCall was always marked cancelled regardless of call status. Fixes grpc#5882
diff --git a/core/src/main/java/io/grpc/internal/ServerCallImpl.java b/core/src/main/java/io/grpc/internal/ServerCallImpl.java
@@ -279,7 +279,11 @@ public ServerStreamListenerImpl(
           new Context.CancellationListener() {
             @Override
             public void cancelled(Context context) {
-              ServerStreamListenerImpl.this.call.cancelled = true;
+              // If the context has a cancellation cause then something exceptional happened
+              // and we should also mark the call as cancelled.
+              if (context.cancellationCause() != null) {
+                ServerStreamListenerImpl.this.call.cancelled = true;
+              }
             }
           },
           MoreExecutors.directExecutor());
@@ -355,6 +359,8 @@ private void closedInternal(Status status) {
       } finally {
         // Cancel context after delivering RPC closure notification to allow the application to
         // clean up and update any state based on whether onComplete or onCancel was called.
+        // Note that in failure situations JumpToApplicationThreadServerStreamListener has already
+        // closed the context. In these situations this cancel() call will be a no-op.
         context.cancel(null);
       }
     }
diff --git a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java
@@ -18,6 +18,7 @@
 
 import static com.google.common.base.Charsets.UTF_8;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -378,6 +379,9 @@ public void streamListener_closedOk() {
     verify(callListener).onComplete();
     assertTrue(context.isCancelled());
     assertNull(context.cancellationCause());
+    // The call considers cancellation to be an exceptional situation so it should
+    // not be cancelled with an OK status.
+    assertFalse(call.isCancelled());
   }
 
   @Test
diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java
@@ -1196,15 +1196,17 @@ public void testStreamClose_clientOkTriggersDelayedCancellation() throws Excepti
         context, contextCancelled, null);
 
     // For close status OK:
-    // isCancelled is expected to be true after all pending work is done
+    // The context isCancelled is expected to be true after all pending work is done,
+    // but for the call it should be false as it gets set cancelled only if the call
+    // fails with a non-OK status.
     assertFalse(callReference.get().isCancelled());
     assertFalse(context.get().isCancelled());
     streamListener.closed(Status.OK);
     assertFalse(callReference.get().isCancelled());
     assertFalse(context.get().isCancelled());
 
     assertEquals(1, executor.runDueTasks());
-    assertTrue(callReference.get().isCancelled());
+    assertFalse(callReference.get().isCancelled());
     assertTrue(context.get().isCancelled());
     assertTrue(contextCancelled.get());
   }
