Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultPromise may throw checked exceptions that are not advertised #8995

Merged
merged 1 commit into from
Apr 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -464,7 +465,11 @@ public void streamClosedErrorTranslatedToClosedChannelExceptionOnWrites() throws

inboundHandler.checkException();

future.syncUninterruptibly();
try {
future.syncUninterruptibly();
} catch (CompletionException e) {
throw e.getCause();
}
}

@Test
Expand Down Expand Up @@ -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());
Expand All @@ -522,7 +527,11 @@ public void failedOutboundStreamCreationThrowsAndClosesChannel() throws Exceptio

handler.checkException();

future.syncUninterruptibly();
try {
future.syncUninterruptibly();
} catch (CompletionException e) {
throw e.getCause();
}
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 8 additions & 0 deletions common/src/main/java/io/netty/util/concurrent/Future.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late, but this should be:

@throws CompletionException if the ....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trustin you mean removing the "package" ?

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Copy link
Contributor

@franz1981 franz1981 Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for readability: import static CoreMatchers.instanceOf
or both is(instanceOf(...

}
Assert.assertTrue(executor.isShutdown());
}
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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");
Expand Down Expand Up @@ -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();
Expand Down
5 changes: 2 additions & 3 deletions handler/src/test/java/io/netty/handler/ssl/SniClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
Expand All @@ -161,6 +162,8 @@ private static void testHandshakeTimeout(boolean client) throws Exception {
}

handler.handshakeFuture().syncUninterruptibly();
} catch (CompletionException e) {
throw e.getCause();
} finally {
ch.finishAndReleaseAll();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading