diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java index 53e6c23550..04b70ee0da 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java @@ -234,8 +234,8 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht weight, exclusive, padding, endOfStream, promise); // Writing headers may fail during the encode state if they violate HPACK limits. - Throwable failureCause = future.cause(); - if (failureCause == null) { + + if (future.isSuccess() || !future.isDone()) { // Synchronously set the headersSent flag to ensure that we do not subsequently write // other headers containing pseudo-header fields. // @@ -248,6 +248,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht notifyLifecycleManagerOnError(future, ctx); } } else { + Throwable failureCause = future.cause(); lifecycleManager.onError(ctx, true, failureCause); } @@ -351,8 +352,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht Future future = frameWriter.writePushPromise(ctx, streamId, promisedStreamId, headers, padding, promise); // Writing headers may fail during the encode state if they violate HPACK limits. - Throwable failureCause = future.cause(); - if (failureCause == null) { + if (future.isSuccess() || !future.isDone()) { // This just sets internal stream state which is used elsewhere in the codec and doesn't // necessarily mean the write will complete successfully. stream.pushPromiseSent(); @@ -362,6 +362,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht notifyLifecycleManagerOnError(future, ctx); } } else { + Throwable failureCause = future.cause(); lifecycleManager.onError(ctx, true, failureCause); } return future; @@ -581,8 +582,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht Future f = sendHeaders(frameWriter, ctx, stream.id(), headers, hasPriority, streamDependency, weight, exclusive, padding, endOfStream, promise); // Writing headers may fail during the encode state if they violate HPACK limits. - Throwable failureCause = f.cause(); - if (failureCause == null) { + if (f.isSuccess() || !f.isDone()) { // This just sets internal stream state which is used elsewhere in the codec and doesn't // necessarily mean the write will complete successfully. stream.headersSent(isInformational); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/StreamBufferingEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/StreamBufferingEncoderTest.java index 1b9d0b297a..a15abaf4d8 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/StreamBufferingEncoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/StreamBufferingEncoderTest.java @@ -256,7 +256,7 @@ public class StreamBufferingEncoderTest { assertEquals(0, encoder.numBufferedStreams()); int failCount = 0; for (Future f : futures) { - if (f.cause() != null) { + if (f.isFailed()) { assertTrue(f.cause() instanceof Http2GoAwayException); failCount++; } @@ -272,7 +272,7 @@ public class StreamBufferingEncoderTest { connection.goAwayReceived(11, 8, EMPTY_BUFFER); Future f = encoderWriteHeaders(5, newPromise()); - assertTrue(f.cause() instanceof Http2GoAwayException); + assertTrue(f.awaitUninterruptibly().cause() instanceof Http2GoAwayException); assertEquals(0, encoder.numBufferedStreams()); } @@ -461,7 +461,7 @@ public class StreamBufferingEncoderTest { Future f = encoderWriteHeaders(-1, newPromise()); // Verify that the write fails. - assertNotNull(f.cause()); + assertNotNull(f.awaitUninterruptibly().cause()); } @Test @@ -493,9 +493,9 @@ public class StreamBufferingEncoderTest { Future f3 = encoderWriteHeaders(7, newPromise()); encoder.close(); - assertNotNull(f1.cause()); - assertNotNull(f2.cause()); - assertNotNull(f3.cause()); + assertNotNull(f1.awaitUninterruptibly().cause()); + assertNotNull(f2.awaitUninterruptibly().cause()); + assertNotNull(f3.awaitUninterruptibly().cause()); } @Test diff --git a/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java b/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java index 04edbf0d02..778db2f28d 100644 --- a/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java +++ b/common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java @@ -398,6 +398,11 @@ public abstract class AbstractScheduledEventExecutor extends AbstractEventExecut return future.isSuccess(); } + @Override + public boolean isFailed() { + return future.isFailed(); + } + @Override public boolean isCancellable() { return future.isCancellable(); diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java index 6f238eb1bb..2203929756 100644 --- a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java +++ b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java @@ -163,6 +163,11 @@ public class DefaultPromise implements Promise { return result != null && result != UNCANCELLABLE && !(result instanceof CauseHolder); } + @Override + public boolean isFailed() { + return result instanceof CauseHolder; + } + @Override public boolean isCancellable() { return result == null; @@ -190,6 +195,9 @@ public class DefaultPromise implements Promise { } private Throwable cause0(Object result) { + if (!isDone0(result)) { + throw new IllegalStateException("Cannot call cause() on a future that has not completed."); + } if (!(result instanceof CauseHolder)) { return null; } @@ -316,7 +324,10 @@ public class DefaultPromise implements Promise { @Override public V getNow() { Object result = this.result; - if (result instanceof CauseHolder || result == SUCCESS || result == UNCANCELLABLE) { + if (!isDone0(result)) { + throw new IllegalStateException("Cannot call getNow() on a future that has not completed."); + } + if (result instanceof CauseHolder || result == SUCCESS) { return null; } return (V) result; diff --git a/common/src/main/java/io/netty/util/concurrent/Future.java b/common/src/main/java/io/netty/util/concurrent/Future.java index a854020d96..1642af7a8e 100644 --- a/common/src/main/java/io/netty/util/concurrent/Future.java +++ b/common/src/main/java/io/netty/util/concurrent/Future.java @@ -53,9 +53,9 @@ import java.util.concurrent.TimeoutException; * | isDone() = false | | +---------------------------+ * | isSuccess() = false |----+----> isDone() = true | * | isCancelled() = false | | | cause() = non-null | - * | cause() = null | | +===========================+ - * +--------------------------+ | | Completed by cancellation | - * | +---------------------------+ + * | cause() = throws | | +===========================+ + * | getNow() = throws | | | Completed by cancellation | + * +--------------------------+ | +---------------------------+ * +----> isDone() = true | * | isCancelled() = true | * +---------------------------+ @@ -168,11 +168,15 @@ import java.util.concurrent.TimeoutException; @SuppressWarnings("ClassNameSameAsAncestorName") public interface Future extends java.util.concurrent.Future { /** - * Returns {@code true} if and only if the I/O operation was completed - * successfully. + * Returns {@code true} if and only if the operation was completed successfully. */ boolean isSuccess(); + /** + * Returns {@code true} if and only if the operation was completed and failed. + */ + boolean isFailed(); + /** * returns {@code true} if and only if the operation can be cancelled via {@link #cancel(boolean)}. */ @@ -183,8 +187,8 @@ public interface Future extends java.util.concurrent.Future { * failed. * * @return the cause of the failure. - * {@code null} if succeeded or this future is not - * completed yet. + * {@code null} if succeeded. + * @throws IllegalStateException if this {@code Future} has not completed yet. */ Throwable cause(); @@ -291,10 +295,9 @@ public interface Future extends java.util.concurrent.Future { boolean awaitUninterruptibly(long timeoutMillis); /** - * Return the result without blocking. If the future is not done yet this will return {@code null}. + * Return the result without blocking. If the future is not done yet this will throw {@link IllegalStateException}. * - * As it is possible that a {@code null} value is used to mark the future as successful you also need to check - * if the future is really done with {@link #isDone()} and not rely on the returned {@code null} value. + * @throws IllegalStateException if this {@code Future} has not completed yet. */ V getNow(); diff --git a/common/src/main/java/io/netty/util/concurrent/RunnableFutureAdapter.java b/common/src/main/java/io/netty/util/concurrent/RunnableFutureAdapter.java index c01497410b..3ac1c4bc68 100644 --- a/common/src/main/java/io/netty/util/concurrent/RunnableFutureAdapter.java +++ b/common/src/main/java/io/netty/util/concurrent/RunnableFutureAdapter.java @@ -44,6 +44,11 @@ final class RunnableFutureAdapter implements RunnableFuture { return promise.isSuccess(); } + @Override + public boolean isFailed() { + return promise.isFailed(); + } + @Override public boolean isCancellable() { return promise.isCancellable(); diff --git a/common/src/main/java/io/netty/util/concurrent/RunnableScheduledFutureAdapter.java b/common/src/main/java/io/netty/util/concurrent/RunnableScheduledFutureAdapter.java index 2db02e2406..0a53a3f373 100644 --- a/common/src/main/java/io/netty/util/concurrent/RunnableScheduledFutureAdapter.java +++ b/common/src/main/java/io/netty/util/concurrent/RunnableScheduledFutureAdapter.java @@ -145,6 +145,11 @@ final class RunnableScheduledFutureAdapter implements AbstractScheduledEventE return promise.isSuccess(); } + @Override + public boolean isFailed() { + return promise.isFailed(); + } + @Override public boolean isCancellable() { return promise.isCancellable(); diff --git a/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java b/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java index 3fe8479ddc..7ff8ac6a18 100644 --- a/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java +++ b/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java @@ -22,7 +22,6 @@ import io.netty.util.internal.logging.InternalLoggerFactory; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; -import org.junit.jupiter.api.function.Executable; import java.util.ArrayList; import java.util.HashMap; @@ -166,6 +165,8 @@ public class DefaultPromiseTest { Exception cause = new Exception(); DefaultPromise promise = new DefaultPromise(executor); promise.setFailure(cause); + assertTrue(promise.isFailed()); + assertFalse(promise.isSuccess()); assertSame(cause, promise.cause()); } @@ -188,6 +189,7 @@ public class DefaultPromiseTest { DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); assertTrue(promise.cancel(false)); assertThat(promise.cause()).isInstanceOf(CancellationException.class); + assertTrue(promise.isFailed()); } @Test @@ -356,6 +358,7 @@ public class DefaultPromiseTest { promise.setSuccess(Signal.valueOf(DefaultPromise.class, "UNCANCELLABLE")); assertTrue(promise.isDone()); assertTrue(promise.isSuccess()); + assertFalse(promise.isFailed()); } @Test @@ -364,21 +367,25 @@ public class DefaultPromiseTest { promise.setSuccess(Signal.valueOf(DefaultPromise.class, "SUCCESS")); assertTrue(promise.isDone()); assertTrue(promise.isSuccess()); + assertFalse(promise.isFailed()); } @Test public void setUncancellableGetNow() { DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); - assertNull(promise.getNow()); + assertThrows(IllegalStateException.class, () -> promise.getNow()); + assertFalse(promise.isDone()); assertTrue(promise.setUncancellable()); - assertNull(promise.getNow()); + assertThrows(IllegalStateException.class, () -> promise.getNow()); assertFalse(promise.isDone()); assertFalse(promise.isSuccess()); + assertFalse(promise.isFailed()); promise.setSuccess("success"); assertTrue(promise.isDone()); assertTrue(promise.isSuccess()); + assertFalse(promise.isFailed()); assertEquals("success", promise.getNow()); } @@ -387,6 +394,7 @@ public class DefaultPromiseTest { Exception exception = new Exception(); DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); promise.setFailure(exception); + assertTrue(promise.isFailed()); try { promise.sync(); @@ -400,6 +408,7 @@ public class DefaultPromiseTest { Exception exception = new Exception(); DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); promise.setFailure(exception); + assertTrue(promise.isFailed()); try { promise.syncUninterruptibly(); @@ -440,6 +449,19 @@ public class DefaultPromiseTest { promise.setSuccess(result); } + @Test + public void getNowOnUnfinishedPromiseMustThrow() { + DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); + assertThrows(IllegalStateException.class, () -> promise.getNow()); + } + + @SuppressWarnings("ThrowableNotThrown") + @Test + public void causeOnUnfinishedPromiseMustThrow() { + DefaultPromise promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE); + assertThrows(IllegalStateException.class, () -> promise.cause()); + } + private static void testStackOverFlowChainedFuturesA(int promiseChainLength, final EventExecutor executor, boolean runTestInExecutorThread) throws InterruptedException { diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index dd065f97e7..5c45be400c 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -828,9 +828,9 @@ public class SslHandler extends ByteToMessageDecoder { if (result.getStatus() == Status.CLOSED) { // Make a best effort to preserve any exception that way previously encountered from the handshake // or the transport, else fallback to a general error. - Throwable exception = handshakePromise.cause(); + Throwable exception = handshakePromise.isDone() ? handshakePromise.cause() : null; if (exception == null) { - exception = sslClosePromise.cause(); + exception = sslClosePromise.isDone() ? sslClosePromise.cause() : null; if (exception == null) { exception = new SslClosedEngineException("SSLEngine closed already"); } @@ -1032,7 +1032,7 @@ public class SslHandler extends ByteToMessageDecoder { @Override public void channelInactive(ChannelHandlerContext ctx) throws Exception { - boolean handshakeFailed = handshakePromise.cause() != null; + boolean handshakeFailed = handshakePromise.isFailed(); ClosedChannelException exception = new ClosedChannelException(); // Make sure to release SSLEngine, diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java index bc3d25cd7f..4fea0cecfa 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java @@ -487,7 +487,7 @@ public class DnsNameResolver extends InetNameResolver { try { ch = b.createUnregistered(); Future future = localAddress == null ? ch.register() : ch.bind(localAddress); - if (future.cause() != null) { + if (future.isFailed()) { throw future.cause(); } } catch (Error | RuntimeException e) { diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java index 5caed7768a..de24fc41e3 100644 --- a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java +++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java @@ -40,6 +40,7 @@ import java.util.concurrent.TimeUnit; import static io.netty.testsuite.transport.socket.SocketTestPermutation.BAD_HOST; import static io.netty.testsuite.transport.socket.SocketTestPermutation.BAD_PORT; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -96,7 +97,7 @@ public class SocketConnectionAttemptTest extends AbstractClientSocketTest { cb.option(ChannelOption.ALLOW_HALF_CLOSURE, halfClosure); Future future = cb.connect(NetUtil.LOCALHOST, UNASSIGNED_PORT).awaitUninterruptibly(); assertThat(future.cause()).isInstanceOf(ConnectException.class); - assertThat(errorPromise.cause()).isNull(); + assertFalse(errorPromise.isFailed()); } @Test diff --git a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java index 1891bd16fd..08b55f1766 100644 --- a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java @@ -240,7 +240,7 @@ public abstract class AbstractBootstrap, C private Future doBind(final SocketAddress localAddress) { EventLoop loop = group.next(); final Future regFuture = initAndRegister(loop); - if (regFuture.cause() != null) { + if (regFuture.isFailed()) { return regFuture; } diff --git a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java index bdcaaf009d..4ee65246a6 100644 --- a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java +++ b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java @@ -1052,7 +1052,7 @@ public class DefaultChannelPipelineTest { assertTrue(handler.addedHandler.get()); assertTrue(handler.removedHandler.get()); assertTrue(handler2.addedHandler.get()); - assertNull(handler2.removedHandler.getNow()); + assertFalse(handler2.removedHandler.isDone()); pipeline.channel().register().syncUninterruptibly(); Throwable cause = handler.error.get(); @@ -1065,7 +1065,7 @@ public class DefaultChannelPipelineTest { throw cause2; } - assertNull(handler2.removedHandler.getNow()); + assertFalse(handler2.removedHandler.isDone()); pipeline.remove(handler2); assertTrue(handler2.removedHandler.get()); pipeline.channel().close().syncUninterruptibly(); @@ -1740,7 +1740,7 @@ public class DefaultChannelPipelineTest { public void handlerAdded(ChannelHandlerContext ctx) throws Exception { if (!addedHandler.trySuccess(true)) { error.set(new AssertionError("handlerAdded(...) called multiple times: " + ctx.name())); - } else if (removedHandler.getNow() == Boolean.TRUE) { + } else if (removedHandler.isDone() && removedHandler.getNow() == Boolean.TRUE) { error.set(new AssertionError("handlerRemoved(...) called before handlerAdded(...): " + ctx.name())); } } @@ -1749,7 +1749,7 @@ public class DefaultChannelPipelineTest { public void handlerRemoved(ChannelHandlerContext ctx) throws Exception { if (!removedHandler.trySuccess(true)) { error.set(new AssertionError("handlerRemoved(...) called multiple times: " + ctx.name())); - } else if (addedHandler.getNow() == Boolean.FALSE) { + } else if (addedHandler.isDone() && addedHandler.getNow() == Boolean.FALSE) { error.set(new AssertionError("handlerRemoved(...) called before handlerAdded(...): " + ctx.name())); } }