From 9b2918e19acfd837f6844a2c00fa6f732bf291a5 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Wed, 11 Aug 2021 11:25:54 -0700 Subject: [PATCH 1/4] Including an exception when cancelling the context when the status is not OK. Also take care to mark the call as cancelled only if the context has a cancellation cause. --- core/src/main/java/io/grpc/internal/ServerCallImpl.java | 8 ++++++-- .../test/java/io/grpc/internal/ServerCallImplTest.java | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ServerCallImpl.java b/core/src/main/java/io/grpc/internal/ServerCallImpl.java index 6f123e76678..bc6a31a165c 100644 --- a/core/src/main/java/io/grpc/internal/ServerCallImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerCallImpl.java @@ -279,7 +279,9 @@ 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. + ServerStreamListenerImpl.this.call.cancelled = context.cancellationCause() != null; } }, MoreExecutors.directExecutor()); @@ -355,7 +357,9 @@ 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. - context.cancel(null); + // Include an exception on any non-OK status to indicate that the context cancellation + // was done under exceptional circumstances. + context.cancel(status.isOk() ? null : status.asRuntimeException()); } } diff --git a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java index 5130bc05aa7..41623552815 100644 --- a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java @@ -19,6 +19,7 @@ import static com.google.common.base.Charsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -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 From 44607a08504bdbcae13e088e708c7934ceb75034 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Wed, 11 Aug 2021 11:25:54 -0700 Subject: [PATCH 2/4] Including an exception when cancelling the context when the status is not OK. Also take care to mark the call as cancelled only if the context has a cancellation cause. --- core/src/main/java/io/grpc/internal/ServerCallImpl.java | 8 +++++++- .../test/java/io/grpc/internal/ServerCallImplTest.java | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/ServerCallImpl.java b/core/src/main/java/io/grpc/internal/ServerCallImpl.java index 6f123e76678..c807426b9f8 100644 --- 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 index 5130bc05aa7..41623552815 100644 --- a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java @@ -19,6 +19,7 @@ import static com.google.common.base.Charsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -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 From 645ae56a879e4d42698c5d1f1d082f9411d09734 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Thu, 12 Aug 2021 09:35:37 -0700 Subject: [PATCH 3/4] Including an exception when cancelling the context when the status is not OK. Also take care to mark the call as cancelled only if the context has a cancellation cause. --- core/src/main/java/io/grpc/internal/ServerCallImpl.java | 4 ++-- core/src/test/java/io/grpc/internal/ServerImplTest.java | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ServerCallImpl.java b/core/src/main/java/io/grpc/internal/ServerCallImpl.java index c807426b9f8..f82d87cade0 100644 --- a/core/src/main/java/io/grpc/internal/ServerCallImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerCallImpl.java @@ -359,8 +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. + // 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/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index 2a9dbd5a1fe..0f5c510f97c 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -1196,7 +1196,9 @@ 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); @@ -1204,7 +1206,7 @@ public void testStreamClose_clientOkTriggersDelayedCancellation() throws Excepti assertFalse(context.get().isCancelled()); assertEquals(1, executor.runDueTasks()); - assertTrue(callReference.get().isCancelled()); + assertFalse(callReference.get().isCancelled()); assertTrue(context.get().isCancelled()); assertTrue(contextCancelled.get()); } From c6f67a468586fb67d1f76dad4d3e7cfebe0386e0 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Fri, 13 Aug 2021 16:41:18 -0700 Subject: [PATCH 4/4] Fix import order in ServerCallImplTest.java. --- core/src/test/java/io/grpc/internal/ServerCallImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java index 41623552815..ea49b94e8aa 100644 --- a/core/src/test/java/io/grpc/internal/ServerCallImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerCallImplTest.java @@ -18,8 +18,8 @@ import static com.google.common.base.Charsets.UTF_8; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any;