From 5b6d8e67725d4fb4fe729f44bcd4ba8d6c0d3cd9 Mon Sep 17 00:00:00 2001 From: Spencer Fang Date: Tue, 26 Sep 2017 14:22:23 -0700 Subject: [PATCH 1/3] context: Make CancellableContext implement Closeable --- context/src/main/java/io/grpc/Context.java | 41 ++++++++++++++++++- .../src/test/java/io/grpc/ContextTest.java | 35 +++++++++++----- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/context/src/main/java/io/grpc/Context.java b/context/src/main/java/io/grpc/Context.java index 910233fae7c..b73797e5a80 100644 --- a/context/src/main/java/io/grpc/Context.java +++ b/context/src/main/java/io/grpc/Context.java @@ -16,6 +16,7 @@ package io.grpc; +import java.io.Closeable; import java.util.ArrayList; import java.util.concurrent.Callable; import java.util.concurrent.Executor; @@ -657,8 +658,35 @@ private Object lookup(Key key) { * cancelled and which will propagate cancellation to its descendants. To avoid leaking memory, * every CancellableContext must have a defined lifetime, after which it is guaranteed to be * cancelled. + * + *

This class must be cleaned up by either calling {@link #close} or {@link #cancel}. + * {@link #close} is equivalent to calling {@code cancel(null)}. It is safe to call the methods + * more than once, but only the first call will have any effect. + * + *

Blocking code can use the try-with-resources idiom: + * 

+   * {@code
+   *
+   *   try (CancellableContext c = Context.current()
+   *       .withDeadlineAfter(100, TimeUnit.MILLISECONDS, executor)) {
+   *     Context toRestore = c.attach();
+   *     try {
+   *       // do some blocking work
+   *     } catch (SomeException e) {
+   *       // Optional: use the caught exception as the cancellation cause.
+   *       // Otherwise the try-with-resources will always call cancel(null) as a part of close().
+   *       c.cancel(e);
+   *     } finally {
+   *       c.detach(toRestore);
+   *     }
+   *   }
+   * }
+   *
+ * + * Asynchronous code will have to manually track the end of the CancellableContext's lifetime, + * and call either {@link #cancel} or {@link #close} at that time. */ - public static final class CancellableContext extends Context { + public static final class CancellableContext extends Context implements Closeable { private final Deadline deadline; private final Context uncancellableSurrogate; @@ -739,7 +767,8 @@ public boolean isCurrent() { /** * Cancel this context and optionally provide a cause (can be {@code null}) for the - * cancellation. This will trigger notification of listeners. + * cancellation. This will trigger notification of listeners. It is safe to call this method + * multiple times. Only the first call will have any effect. * * @return {@code true} if this context cancelled the context and notified listeners, * {@code false} if the context was already cancelled. @@ -811,6 +840,14 @@ public Deadline getDeadline() { boolean canBeCancelled() { return true; } + + /** + * Cleans up this object by calling {@code cancel(null)}. + */ + @Override + public void close() { + cancel(null); + } } /** diff --git a/context/src/test/java/io/grpc/ContextTest.java b/context/src/test/java/io/grpc/ContextTest.java index 93191bdf2fe..4f8e082ab6e 100644 --- a/context/src/test/java/io/grpc/ContextTest.java +++ b/context/src/test/java/io/grpc/ContextTest.java @@ -95,13 +95,14 @@ public void setUp() throws Exception { @After public void tearDown() throws Exception { scheduler.shutdown(); + assertEquals(Context.ROOT, Context.current()); } @Test public void defaultContext() throws Exception { final SettableFuture contextOfNewThread = SettableFuture.create(); Context contextOfThisThread = Context.ROOT.withValue(PET, "dog"); - contextOfThisThread.attach(); + Context toRestore = contextOfThisThread.attach(); new Thread(new Runnable() { @Override public void run() { @@ -111,16 +112,22 @@ public void run() { assertNotNull(contextOfNewThread.get(5, TimeUnit.SECONDS)); assertNotSame(contextOfThisThread, contextOfNewThread.get()); assertSame(contextOfThisThread, Context.current()); + contextOfThisThread.detach(toRestore); } @Test public void rootCanBeAttached() { Context fork = Context.ROOT.fork(); - fork.attach(); - Context.ROOT.attach(); + Context toRestore1 = fork.attach(); + Context toRestore2 = Context.ROOT.attach(); assertTrue(Context.ROOT.isCurrent()); - fork.attach(); + + Context toRestore3 = fork.attach(); assertTrue(fork.isCurrent()); + + fork.detach(toRestore3); + Context.ROOT.detach(toRestore2); + fork.detach(toRestore1); } @Test @@ -225,14 +232,14 @@ public void withValuesThree() { Context base = Context.current().withValues(PET, "dog", COLOR, "blue"); Context child = base.withValues(PET, "cat", FOOD, "cheese", FAVORITE, fav); - child.attach(); + Context toRestore = child.attach(); assertEquals("cat", PET.get()); assertEquals("cheese", FOOD.get()); assertEquals("blue", COLOR.get()); assertEquals(fav, FAVORITE.get()); - base.attach(); + child.detach(toRestore); } @Test @@ -241,7 +248,7 @@ public void withValuesFour() { Context base = Context.current().withValues(PET, "dog", COLOR, "blue"); Context child = base.withValues(PET, "cat", FOOD, "cheese", FAVORITE, fav, LUCKY, 7); - child.attach(); + Context toRestore = child.attach(); assertEquals("cat", PET.get()); assertEquals("cheese", FOOD.get()); @@ -249,7 +256,7 @@ public void withValuesFour() { assertEquals(fav, FAVORITE.get()); assertEquals(7, (int) LUCKY.get()); - base.attach(); + child.detach(toRestore); } @Test @@ -389,7 +396,7 @@ public void cascadingCancellationWithoutListener() { public void cancellableContextIsAttached() { Context.CancellableContext base = Context.current().withValue(FOOD, "fish").withCancellation(); assertFalse(base.isCurrent()); - base.attach(); + Context toRestore = base.attach(); Context attached = Context.current(); assertSame("fish", FOOD.get()); @@ -406,7 +413,7 @@ public void cancellableContextIsAttached() { assertSame(t, attached.cancellationCause()); assertSame(attached, listenerNotifedContext); - Context.ROOT.attach(); + base.detach(toRestore); } @Test @@ -921,6 +928,14 @@ public void cancellableAncestorFork() { assertNull(fork.cancellableAncestor); } + @Test + public void cancellableContext_closeCancelsWithNullCause() throws Exception { + Context.CancellableContext cancellable = Context.current().withCancellation(); + cancellable.close(); + assertTrue(cancellable.isCancelled()); + assertNull(cancellable.cancellationCause()); + } + @Test public void errorWhenAncestryLengthLong() { final AtomicReference logRef = new AtomicReference(); From f4c6b14452111736228560f5daa8f0b4c76229fb Mon Sep 17 00:00:00 2001 From: Spencer Fang Date: Mon, 23 Oct 2017 19:47:39 -0700 Subject: [PATCH 2/3] fix checkstyle --- context/src/main/java/io/grpc/Context.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/context/src/main/java/io/grpc/Context.java b/context/src/main/java/io/grpc/Context.java index b73797e5a80..407928c567e 100644 --- a/context/src/main/java/io/grpc/Context.java +++ b/context/src/main/java/io/grpc/Context.java @@ -683,7 +683,7 @@ private Object lookup(Key key) { * } * * - * Asynchronous code will have to manually track the end of the CancellableContext's lifetime, + *

Asynchronous code will have to manually track the end of the CancellableContext's lifetime, * and call either {@link #cancel} or {@link #close} at that time. */ public static final class CancellableContext extends Context implements Closeable { From ffbf25616c7e872885d576b7c24f09d7c9ed4bde Mon Sep 17 00:00:00 2001 From: Spencer Fang Date: Wed, 25 Oct 2017 09:37:02 -0700 Subject: [PATCH 3/3] tweak javadoc --- context/src/main/java/io/grpc/Context.java | 34 ++++++++++------------ 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/context/src/main/java/io/grpc/Context.java b/context/src/main/java/io/grpc/Context.java index 407928c567e..4734e854578 100644 --- a/context/src/main/java/io/grpc/Context.java +++ b/context/src/main/java/io/grpc/Context.java @@ -659,32 +659,26 @@ private Object lookup(Key key) { * every CancellableContext must have a defined lifetime, after which it is guaranteed to be * cancelled. * - *

This class must be cleaned up by either calling {@link #close} or {@link #cancel}. + *

This class must be cancelled by either calling {@link #close} or {@link #cancel}. * {@link #close} is equivalent to calling {@code cancel(null)}. It is safe to call the methods - * more than once, but only the first call will have any effect. + * more than once, but only the first call will have any effect. Because it's safe to call the + * methods multiple times, users are encouraged to always call {@link #close} at the end of + * the operation, and disregard whether {@link #cancel} was already called somewhere else. * *

Blocking code can use the try-with-resources idiom: * 

-   * {@code
-   *
-   *   try (CancellableContext c = Context.current()
-   *       .withDeadlineAfter(100, TimeUnit.MILLISECONDS, executor)) {
-   *     Context toRestore = c.attach();
-   *     try {
-   *       // do some blocking work
-   *     } catch (SomeException e) {
-   *       // Optional: use the caught exception as the cancellation cause.
-   *       // Otherwise the try-with-resources will always call cancel(null) as a part of close().
-   *       c.cancel(e);
-   *     } finally {
-   *       c.detach(toRestore);
-   *     }
+   * try (CancellableContext c = Context.current()
+   *     .withDeadlineAfter(100, TimeUnit.MILLISECONDS, executor)) {
+   *   Context toRestore = c.attach();
+   *   try {
+   *     // do some blocking work
+   *   } finally {
+   *     c.detach(toRestore);
    *   }
-   * }
-   *
+ * } * *

Asynchronous code will have to manually track the end of the CancellableContext's lifetime, - * and call either {@link #cancel} or {@link #close} at that time. + * and cancel the context at the appropriate time. */ public static final class CancellableContext extends Context implements Closeable { @@ -770,6 +764,8 @@ public boolean isCurrent() { * cancellation. This will trigger notification of listeners. It is safe to call this method * multiple times. Only the first call will have any effect. * + *

Calling {@code cancel(null)} is the same as calling {@link #close}. + * * @return {@code true} if this context cancelled the context and notified listeners, * {@code false} if the context was already cancelled. */