DefaultPromise may throw checked exceptions that are not advertised

Motivation: We should not throw check exceptions when the user calls sync*() but should better wrap it in a CompletionException to make it easier for people to reason about what happens. Modifications: - Change sync*() to throw CompletionException - Adjust tests - Add some more tests Result: Fixes #8521.
netty · Apr 1, 2019 · fea799c · fea799c
1 parent 07244a1
commit fea799c
Show file tree

Hide file tree

Showing 16 changed files with 152 additions and 52 deletions.
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java
@@ -40,6 +40,7 @@
 import java.nio.channels.ClosedChannelException;
 import java.util.ArrayDeque;
 import java.util.Queue;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -443,7 +444,7 @@ public void streamExceptionTriggersChildChannelExceptionAndClose() throws Except
     }
 
     @Test(expected = ClosedChannelException.class)
-    public void streamClosedErrorTranslatedToClosedChannelExceptionOnWrites() throws Exception {
+    public void streamClosedErrorTranslatedToClosedChannelExceptionOnWrites() throws Throwable {
         LastInboundHandler inboundHandler = new LastInboundHandler();
 
         final Http2StreamChannel childChannel = newOutboundStream(inboundHandler);
@@ -464,7 +465,11 @@ public void streamClosedErrorTranslatedToClosedChannelExceptionOnWrites() throws
 
         inboundHandler.checkException();
 
-        future.syncUninterruptibly();
+        try {
+            future.syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test
@@ -502,7 +507,7 @@ public void creatingWritingReadingAndClosingOutboundStreamShouldWork() {
     // likely happen due to the max concurrent streams limit being hit or the channel running out of stream identifiers.
     //
     @Test(expected = Http2NoMoreStreamIdsException.class)
-    public void failedOutboundStreamCreationThrowsAndClosesChannel() throws Exception {
+    public void failedOutboundStreamCreationThrowsAndClosesChannel() throws Throwable {
         LastInboundHandler handler = new LastInboundHandler();
         Http2StreamChannel childChannel = newOutboundStream(handler);
         assertTrue(childChannel.isActive());
@@ -522,7 +527,11 @@ public void failedOutboundStreamCreationThrowsAndClosesChannel() throws Exceptio
 
         handler.checkException();
 
-        future.syncUninterruptibly();
+        try {
+            future.syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test

diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
@@ -24,6 +24,7 @@
 import io.netty.util.internal.logging.InternalLoggerFactory;
 
 import java.util.concurrent.CancellationException;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 
@@ -561,8 +562,10 @@ private void rethrowIfFailed() {
         if (cause == null) {
             return;
         }
-
-        PlatformDependent.throwException(cause);
+        if (cause instanceof CancellationException) {
+            throw (CancellationException) cause;
+        }
+        throw new CompletionException(cause);
     }
 
     private boolean await0(long timeoutNanos, boolean interruptable) throws InterruptedException {

diff --git a/common/src/main/java/io/netty/util/concurrent/Future.java b/common/src/main/java/io/netty/util/concurrent/Future.java
@@ -83,12 +83,20 @@ public interface Future<V> extends java.util.concurrent.Future<V> {
     /**
      * Waits for this future until it is done, and rethrows the cause of the failure if this future
      * failed.
+     *
+     * @throws CancellationException if the computation was cancelled
+     * @throws {@link java.util.concurrent.CompletionException} if the computation threw an exception.
+     * @throws InterruptedException if the current thread was interrupted while waiting
+     *
      */
     Future<V> sync() throws InterruptedException;
 
     /**
      * Waits for this future until it is done, and rethrows the cause of the failure if this future
      * failed.
+     *
+     * @throws CancellationException if the computation was cancelled
+     * @throws {@link java.util.concurrent.CompletionException} if the computation threw an exception.
      */
     Future<V> syncUninterruptibly();
 

diff --git a/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java b/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java
@@ -27,6 +27,7 @@
 import java.util.Map;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.CancellationException;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
@@ -297,6 +298,39 @@ public void setUncancellableGetNow() {
         assertEquals("success", promise.getNow());
     }
 
+    @Test
+    public void throwUncheckedSync() throws InterruptedException {
+        Exception exception = new Exception();
+        final Promise<String> promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE);
+        promise.setFailure(exception);
+
+        try {
+            promise.sync();
+        } catch (CompletionException e) {
+            assertSame(exception, e.getCause());
+        }
+    }
+
+    @Test
+    public void throwUncheckedSyncUninterruptibly() {
+        Exception exception = new Exception();
+        final Promise<String> promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE);
+        promise.setFailure(exception);
+
+        try {
+            promise.syncUninterruptibly();
+        } catch (CompletionException e) {
+            assertSame(exception, e.getCause());
+        }
+    }
+
+    @Test(expected = CancellationException.class)
+    public void throwCancelled() throws InterruptedException {
+        final Promise<String> promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE);
+        promise.cancel(true);
+        promise.sync();
+    }
+
     private static void testStackOverFlowChainedFuturesA(int promiseChainLength, final EventExecutor executor,
                                                          boolean runTestInExecutorThread)
             throws InterruptedException {

diff --git a/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java b/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java
@@ -15,12 +15,14 @@
  */
 package io.netty.util.concurrent;
 
+import org.hamcrest.CoreMatchers;
 import org.junit.Assert;
 import org.junit.Test;
 
 import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -52,8 +54,9 @@ protected void run() {
         try {
             executor.shutdownGracefully().syncUninterruptibly();
             Assert.fail();
-        } catch (RejectedExecutionException expected) {
+        } catch (CompletionException expected) {
             // expected
+            Assert.assertThat(expected.getCause(), CoreMatchers.instanceOf(RejectedExecutionException.class));
         }
         Assert.assertTrue(executor.isShutdown());
     }
@@ -97,23 +100,39 @@ protected void run() {
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAnyInEventLoop() {
-        testInvokeInEventLoop(true, false);
+    public void testInvokeAnyInEventLoop() throws Throwable {
+        try {
+            testInvokeInEventLoop(true, false);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAnyInEventLoopWithTimeout() {
-        testInvokeInEventLoop(true, true);
+    public void testInvokeAnyInEventLoopWithTimeout() throws Throwable {
+        try {
+            testInvokeInEventLoop(true, true);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAllInEventLoop() {
-        testInvokeInEventLoop(false, false);
+    public void testInvokeAllInEventLoop() throws Throwable {
+        try {
+            testInvokeInEventLoop(false, false);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAllInEventLoopWithTimeout() {
-        testInvokeInEventLoop(false, true);
+    public void testInvokeAllInEventLoopWithTimeout() throws Throwable {
+        try {
+            testInvokeInEventLoop(false, true);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     private static void testInvokeInEventLoop(final boolean any, final boolean timeout) {

diff --git a/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java b/handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java
@@ -67,6 +67,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.CompletionException;
 
 /**
  * In extra class to be able to run tests with java7 without trying to load classes that not exists in java7.
@@ -76,7 +77,7 @@ final class SniClientJava8TestUtil {
     private SniClientJava8TestUtil() { }
 
     static void testSniClient(SslProvider sslClientProvider, SslProvider sslServerProvider, final boolean match)
-            throws Exception {
+            throws Throwable {
         final String sniHost = "sni.netty.io";
         SelfSignedCertificate cert = new SelfSignedCertificate();
         LocalAddress address = new LocalAddress("test");
@@ -150,6 +151,8 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
 
             promise.syncUninterruptibly();
             sslHandler.handshakeFuture().syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             if (cc != null) {
                 cc.close().syncUninterruptibly();

diff --git a/handler/src/test/java/io/netty/handler/ssl/SniClientTest.java b/handler/src/test/java/io/netty/handler/ssl/SniClientTest.java
@@ -27,7 +27,6 @@
 import io.netty.channel.local.LocalHandler;
 import io.netty.channel.local.LocalServerChannel;
 import io.netty.handler.ssl.util.SelfSignedCertificate;
-import io.netty.util.Mapping;
 import io.netty.util.ReferenceCountUtil;
 import io.netty.util.concurrent.Promise;
 
@@ -80,12 +79,12 @@ public void testSniClient() throws Exception {
     }
 
     @Test(timeout = 30000)
-    public void testSniSNIMatcherMatchesClient() throws Exception {
+    public void testSniSNIMatcherMatchesClient() throws Throwable {
         SniClientJava8TestUtil.testSniClient(serverProvider, clientProvider, true);
     }
 
     @Test(timeout = 30000, expected = SSLException.class)
-    public void testSniSNIMatcherDoesNotMatchClient() throws Exception {
+    public void testSniSNIMatcherDoesNotMatchClient() throws Throwable {
         SniClientJava8TestUtil.testSniClient(serverProvider, clientProvider, false);
     }
 

diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -61,6 +61,7 @@
 import java.nio.channels.ClosedChannelException;
 import java.security.NoSuchAlgorithmException;
 import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
@@ -127,12 +128,12 @@ public boolean isActive() {
     }
 
     @Test(expected = SSLException.class, timeout = 3000)
-    public void testClientHandshakeTimeout() throws Exception {
+    public void testClientHandshakeTimeout() throws Throwable {
         testHandshakeTimeout(true);
     }
 
     @Test(expected = SSLException.class, timeout = 3000)
-    public void testServerHandshakeTimeout() throws Exception {
+    public void testServerHandshakeTimeout() throws Throwable {
         testHandshakeTimeout(false);
     }
 
@@ -146,7 +147,7 @@ private static SSLEngine newServerModeSSLEngine() throws NoSuchAlgorithmExceptio
         return engine;
     }
 
-    private static void testHandshakeTimeout(boolean client) throws Exception {
+    private static void testHandshakeTimeout(boolean client) throws Throwable {
         SSLEngine engine = SSLContext.getDefault().createSSLEngine();
         engine.setUseClientMode(client);
         SslHandler handler = new SslHandler(engine);
@@ -161,6 +162,8 @@ private static void testHandshakeTimeout(boolean client) throws Exception {
             }
 
             handler.handshakeFuture().syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             ch.finishAndReleaseAll();
         }

diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
@@ -83,6 +83,7 @@
 import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.Set;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -679,10 +680,11 @@ public void run() {
 
     private static UnknownHostException resolveNonExistentDomain(DnsNameResolver resolver) {
         try {
-            resolver.resolve("non-existent.netty.io").sync();
+            resolver.resolve("non-existent.netty.io").syncUninterruptibly();
             fail();
             return null;
-        } catch (Exception e) {
+        } catch (CompletionException cause) {
+            Throwable e = cause.getCause();
             assertThat(e, is(instanceOf(UnknownHostException.class)));
 
             TestRecursiveCacheDnsQueryLifecycleObserverFactory lifecycleObserverFactory =
@@ -2108,7 +2110,7 @@ public void testFollowCNAMEEvenIfARecordIsPresent() throws IOException {
     }
 
     @Test
-    public void testFollowCNAMELoop() throws IOException {
+    public void testFollowCNAMELoop() throws Throwable {
         expectedException.expect(UnknownHostException.class);
         TestDnsServer dnsServer2 = new TestDnsServer(question -> {
             Set<ResourceRecord> records = new LinkedHashSet<>(4);
@@ -2141,6 +2143,8 @@ public void testFollowCNAMELoop() throws IOException {
 
             resolver = builder.build();
             resolver.resolveAll("somehost.netty.io").syncUninterruptibly().getNow();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             dnsServer2.stop();
             if (resolver != null) {
@@ -2150,24 +2154,26 @@ public void testFollowCNAMELoop() throws IOException {
     }
 
     @Test
-    public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() {
+    public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() throws Throwable {
         expectedException.expect(UnknownHostException.class);
         testSearchDomainQueryFailureCompletes(ResolvedAddressTypes.IPV4_ONLY);
     }
 
     @Test
-    public void testSearchDomainQueryFailureForMultipleAddressTypeCompletes() {
+    public void testSearchDomainQueryFailureForMultipleAddressTypeCompletes() throws Throwable {
         expectedException.expect(UnknownHostException.class);
         testSearchDomainQueryFailureCompletes(ResolvedAddressTypes.IPV4_PREFERRED);
     }
 
-    private void testSearchDomainQueryFailureCompletes(ResolvedAddressTypes types) {
+    private void testSearchDomainQueryFailureCompletes(ResolvedAddressTypes types) throws Throwable {
         DnsNameResolver resolver = newResolver()
                 .resolvedAddressTypes(types)
                 .ndots(1)
                 .searchDomains(singletonList(".")).build();
         try {
             resolver.resolve("invalid.com").syncUninterruptibly();
+        } catch (CompletionException cause) {
+            throw cause.getCause();
         } finally {
             resolver.close();
         }

diff --git a/...e/src/main/java/io/netty/testsuite/transport/socket/SocketChannelNotYetConnectedTest.java b/...e/src/main/java/io/netty/testsuite/transport/socket/SocketChannelNotYetConnectedTest.java
@@ -40,14 +40,14 @@ public void testShutdownNotYetConnected(Bootstrap cb) throws Throwable {
                 ch.shutdownInput().syncUninterruptibly();
                 fail();
             } catch (Throwable cause) {
-                checkThrowable(cause);
+                checkThrowable(cause.getCause());
             }
 
             try {
                 ch.shutdownOutput().syncUninterruptibly();
                 fail();
             } catch (Throwable cause) {
-                checkThrowable(cause);
+                checkThrowable(cause.getCause());
             }
         } finally {
             ch.close().syncUninterruptibly();