DefaultPromise may throw checked exceptions that are not advertised (#8995)

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 https://github.com/netty/netty/issues/8521.

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java

@@ -40,6 +40,7 @@ import java.net.InetSocketAddress;
 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 class Http2MultiplexCodecTest {
     }
 
     @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 class Http2MultiplexCodecTest {
 
         inboundHandler.checkException();
 
+        try {
             future.syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test
@@ -502,7 +507,7 @@ public class Http2MultiplexCodecTest {
     // 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 class Http2MultiplexCodecTest {
 
         handler.checkException();
 
+        try {
             future.syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

@@ -24,6 +24,7 @@ import io.netty.util.internal.logging.InternalLogger;
 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;
 
@@ -546,8 +547,10 @@ public class DefaultPromise<V> extends AbstractFuture<V> implements Promise<V> {
         if (cause == null) {
             return;
         }
-
+        if (cause instanceof CancellationException) {
-        PlatformDependent.throwException(cause);
+            throw (CancellationException) cause;
+        }
+        throw new CompletionException(cause);
     }
 
     private boolean await0(long timeoutNanos, boolean interruptable) throws InterruptedException {

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();
 

common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java

@@ -27,6 +27,7 @@ import java.util.HashMap;
 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 class DefaultPromiseTest {
         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 {

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 @@ public class SingleThreadEventExecutorTest {
         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 @@ public class SingleThreadEventExecutorTest {
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAnyInEventLoop() {
+    public void testInvokeAnyInEventLoop() throws Throwable {
+        try {
             testInvokeInEventLoop(true, false);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAnyInEventLoopWithTimeout() {
+    public void testInvokeAnyInEventLoopWithTimeout() throws Throwable {
+        try {
             testInvokeInEventLoop(true, true);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAllInEventLoop() {
+    public void testInvokeAllInEventLoop() throws Throwable {
+        try {
             testInvokeInEventLoop(false, false);
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test(expected = RejectedExecutionException.class, timeout = 3000)
-    public void testInvokeAllInEventLoopWithTimeout() {
+    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) {

handler/src/test/java/io/netty/handler/ssl/SniClientJava8TestUtil.java

@@ -67,6 +67,7 @@ import java.security.cert.X509Certificate;
 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 @@ final class SniClientJava8TestUtil {
 
             promise.syncUninterruptibly();
             sslHandler.handshakeFuture().syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             if (cc != null) {
                 cc.close().syncUninterruptibly();

handler/src/test/java/io/netty/handler/ssl/SniClientTest.java

@@ -27,7 +27,6 @@ import io.netty.channel.local.LocalChannel;
 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 class SniClientTest {
     }
 
     @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);
     }
 

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

@@ -61,6 +61,7 @@ import java.net.InetSocketAddress;
 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 class SslHandlerTest {
     }
 
     @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 @@ public class SslHandlerTest {
         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 @@ public class SslHandlerTest {
             }
 
             handler.handshakeFuture().syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             ch.finishAndReleaseAll();
         }

resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java

@@ -83,6 +83,7 @@ import java.util.Map;
 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 class DnsNameResolverTest {
 
     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 class DnsNameResolverTest {
     }
 
     @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 class DnsNameResolverTest {
 
             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 class DnsNameResolverTest {
     }
 
     @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();
         }

testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketChannelNotYetConnectedTest.java

@@ -40,14 +40,14 @@ public class SocketChannelNotYetConnectedTest extends AbstractClientSocketTest {
                 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();

testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketShutdownOutputBySelfTest.java

@@ -113,13 +113,13 @@ public class SocketShutdownOutputBySelfTest extends AbstractClientSocketTest {
                 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 {
             if (s != null) {
@@ -177,7 +177,7 @@ public class SocketShutdownOutputBySelfTest extends AbstractClientSocketTest {
                 ch.writeAndFlush(Unpooled.wrappedBuffer(new byte[]{ 2 })).sync();
                 fail();
             } catch (Throwable cause) {
-                checkThrowable(cause);
+                checkThrowable(cause.getCause());
             }
             assertNull(h.writabilityQueue.poll());
         } finally {

transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollReuseAddrTest.java

@@ -84,7 +84,7 @@ public class EpollReuseAddrTest {
             bootstrap.bind(future.channel().localAddress()).syncUninterruptibly();
             Assert.fail();
         } catch (Exception e) {
-            Assert.assertTrue(e instanceof IOException);
+            Assert.assertTrue(e.getCause() instanceof IOException);
         }
         future.channel().close().syncUninterruptibly();
     }

transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketTcpMd5Test.java

@@ -26,6 +26,7 @@ import io.netty.util.CharsetUtil;
 
 import java.net.InetSocketAddress;
 import java.util.Collections;
+import java.util.concurrent.CompletionException;
 
 import io.netty.util.NetUtil;
 import org.junit.After;
@@ -87,18 +88,23 @@ public class EpollSocketTcpMd5Test {
     }
 
     @Test(expected = ConnectTimeoutException.class)
-    public void testKeyMismatch() throws Exception {
+    public void testKeyMismatch() throws Throwable {
         server.config().setOption(EpollChannelOption.TCP_MD5SIG,
                 Collections.singletonMap(NetUtil.LOCALHOST4, SERVER_KEY));
 
+        try {
             EpollSocketChannel client = (EpollSocketChannel) new Bootstrap().group(GROUP)
                     .channel(EpollSocketChannel.class)
-                .handler(new ChannelHandler() { })
+                    .handler(new ChannelHandler() {
+                    })
                     .option(EpollChannelOption.TCP_MD5SIG,
                             Collections.singletonMap(NetUtil.LOCALHOST4, BAD_KEY))
                     .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, 1000)
                     .connect(server.localAddress()).syncUninterruptibly().channel();
             client.close().syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     @Test

transport/src/test/java/io/netty/bootstrap/BootstrapTest.java

@@ -45,6 +45,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.LinkedBlockingQueue;
 
@@ -203,7 +204,7 @@ public class BootstrapTest {
     }
 
     @Test(expected = ConnectException.class, timeout = 10000)
-    public void testLateRegistrationConnect() throws Exception {
+    public void testLateRegistrationConnect() throws Throwable {
         EventLoopGroup group = new MultithreadEventLoopGroup(1, LocalHandler.newFactory());
         LateRegisterHandler registerHandler = new LateRegisterHandler();
         try {
@@ -215,6 +216,8 @@ public class BootstrapTest {
             assertFalse(future.isDone());
             registerHandler.registerPromise().setSuccess();
             future.syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
         } finally {
             group.shutdownGracefully();
         }

transport/src/test/java/io/netty/channel/ReentrantChannelTest.java

@@ -20,6 +20,7 @@ import io.netty.bootstrap.ServerBootstrap;
 import io.netty.channel.LoggingHandler.Event;
 import io.netty.channel.local.LocalAddress;
 
+import org.hamcrest.Matchers;
 import org.junit.Test;
 
 import java.nio.channels.ClosedChannelException;
@@ -270,7 +271,7 @@ public class ReentrantChannelTest extends BaseChannelTest {
             fail();
         } catch (Throwable cce) {
             // FIXME:  shouldn't this contain the "intentional failure" exception?
-            assertEquals(ClosedChannelException.class, cce.getClass());
+            assertThat(cce.getCause(), Matchers.instanceOf(ClosedChannelException.class));
         }
 
         clientChannel.closeFuture().sync();

transport/src/test/java/io/netty/channel/local/LocalChannelTest.java

@@ -46,6 +46,7 @@ import org.junit.Test;
 
 import java.net.ConnectException;
 import java.nio.channels.ClosedChannelException;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
@@ -171,7 +172,8 @@ public class LocalChannelTest {
             try {
                 cc.writeAndFlush(new Object()).sync();
                 fail("must raise a ClosedChannelException");
-            } catch (Exception e) {
+            } catch (CompletionException cause) {
+                Throwable e = cause.getCause();
                 assertThat(e, is(instanceOf(ClosedChannelException.class)));
                 // Ensure that the actual write attempt on a closed channel was never made by asserting that
                 // the ClosedChannelException has been created by AbstractUnsafe rather than transport implementations.
@@ -814,12 +816,16 @@ public class LocalChannelTest {
     }
 
     @Test(expected = ConnectException.class)
-    public void testConnectionRefused() {
+    public void testConnectionRefused() throws Throwable {
+        try {
             Bootstrap sb = new Bootstrap();
             sb.group(group1)
                     .channel(LocalChannel.class)
                     .handler(new TestHandler())
                     .connect(LocalAddress.ANY).syncUninterruptibly();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        }
     }
 
     private static final class LatchChannelFutureListener extends CountDownLatch implements ChannelFutureListener {