Change `!future.isSuccess()` to `future.isFailed()` where it makes sense (#11616)

Motivation:
The expression "not is success" can mean that either the future failed, or it has not yet completed.
However, many places where such an expression is used is expecting the future to have completed.
Specifically, they are expecting to be able to call `cause()` on the future.
It is both more correct, and semantically clearer, to call `isFailed()` instead of `!isSuccess()`.

Modification:
Change all places that used `!isSuccess()` to  mean that the future had failed, to use `isFailed()`.
A few places are relying on `isSuccess()` returning `false` for _incomplete_ futures, and these places have been left unchanged.

Result:
Clearer code, with potentially fewer latent bugs.
This commit is contained in:
Chris Vest 2021-08-26 09:43:17 +02:00 committed by GitHub
parent 2a7cc36536
commit edf4e28afa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
31 changed files with 36 additions and 35 deletions

View File

@ -233,7 +233,7 @@ public class HAProxyMessageDecoderTest {
// swallow this exception since we're just testing to be sure the channel was closed
}
boolean isComplete = closeFuture.awaitUninterruptibly(5000);
if (!isComplete || !closeFuture.isDone() || !closeFuture.isSuccess()) {
if (!isComplete || !closeFuture.isDone() || closeFuture.isFailed()) {
fail("Expected channel close");
}
}

View File

@ -251,14 +251,14 @@ public class HttpObjectAggregator
!HttpUtil.is100ContinueExpected(oversized) && !HttpUtil.isKeepAlive(oversized)) {
Future<Void> future = ctx.writeAndFlush(TOO_LARGE_CLOSE.retainedDuplicate());
future.addListener(future1 -> {
if (!future1.isSuccess()) {
if (future1.isFailed()) {
logger.debug("Failed to send a 413 Request Entity Too Large.", future1.cause());
}
ctx.close();
});
} else {
ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
logger.debug("Failed to send a 413 Request Entity Too Large.", future.cause());
ctx.close();
}

View File

@ -54,7 +54,7 @@ class WebSocketClientProtocolHandshakeHandler implements ChannelHandler {
public void channelActive(final ChannelHandlerContext ctx) throws Exception {
ctx.fireChannelActive();
handshaker.handshake(ctx.channel()).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
handshakePromise.tryFailure(future.cause());
ctx.fireExceptionCaught(future.cause());
} else {

View File

@ -86,7 +86,7 @@ class WebSocketServerProtocolHandshakeHandler implements ChannelHandler {
Future<Void> handshakeFuture = handshaker.handshake(ctx.channel(), req);
handshakeFuture.addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
localHandshakePromise.tryFailure(future.cause());
ctx.fireExceptionCaught(future.cause());
} else {

View File

@ -583,7 +583,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht
Future<Void> 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.
if (f.isSuccess() || !f.isDone()) {
if (!f.isFailed()) { // "not failed" means either not done, or completed successfully.
// 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);
@ -623,7 +623,7 @@ public class DefaultHttp2ConnectionEncoder implements Http2ConnectionEncoder, Ht
@Override
public void operationComplete(Future<? extends Void> future) throws Exception {
if (!future.isSuccess()) {
if (future.isFailed()) {
error(flowController().channelHandlerContext(), future.cause());
}
}

View File

@ -842,7 +842,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
private void closeConnectionOnError(ChannelHandlerContext ctx, Future<?> future) {
if (!future.isSuccess()) {
if (future.isFailed()) {
onConnectionError(ctx, true, future.cause(), null);
}
}

View File

@ -463,7 +463,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
}
private void handleHeaderFuture(Future<?> channelFuture, int streamId) {
if (!channelFuture.isSuccess()) {
if (channelFuture.isFailed()) {
frameStreamToInitializeMap.remove(streamId);
}
}

View File

@ -129,7 +129,7 @@ public final class Http2MultiplexHandler extends Http2ChannelDuplexHandler {
// Handle any errors that occurred on the local thread while registering. Even though
// failures can happen after this point, they will be handled by the channel by closing the
// childChannel.
if (!future.isSuccess()) {
if (future.isFailed()) {
if (childChannel.isRegistered()) {
childChannel.close();
} else {

View File

@ -225,7 +225,7 @@ public abstract class MessageAggregator<I, S, C extends ByteBufHolder, O extends
FutureContextListener<ChannelHandlerContext, Void> listener = continueResponseWriteListener;
if (listener == null) {
continueResponseWriteListener = listener = (context, future) -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
context.fireExceptionCaught(future.cause());
}
};

View File

@ -51,7 +51,7 @@ public final class PromiseCombiner {
private void operationComplete0(Future<?> future) {
assert executor.inEventLoop();
++doneCount;
if (!future.isSuccess() && cause == null) {
if (future.isFailed() && cause == null) {
cause = future.cause();
}
if (doneCount == expectedCount && aggregatePromise != null) {

View File

@ -215,6 +215,7 @@ public class PromiseCombinerTest {
private static void mockFailedPromise(Promise<Void> p, Throwable cause, FutureListenerConsumer consumer) {
when(p.isDone()).thenReturn(true);
when(p.isSuccess()).thenReturn(false);
when(p.isFailed()).thenReturn(true);
when(p.cause()).thenReturn(cause);
mockListener(p, consumer);
}

View File

@ -48,7 +48,7 @@ public class Http2SettingsHandler extends SimpleChannelInboundHandler<Http2Setti
if (!promise.awaitUninterruptibly(timeout, unit)) {
throw new IllegalStateException("Timed out waiting for settings");
}
if (!promise.isSuccess()) {
if (promise.isFailed()) {
throw new RuntimeException(promise.cause());
}
}

View File

@ -71,14 +71,14 @@ public class HttpResponseHandler extends SimpleChannelInboundHandler<FullHttpRes
if (!writeFuture.awaitUninterruptibly(timeout, unit)) {
throw new IllegalStateException("Timed out waiting to write for stream id " + entry.getKey());
}
if (!writeFuture.isSuccess()) {
if (writeFuture.isFailed()) {
throw new RuntimeException(writeFuture.cause());
}
Promise<Void> promise = entry.getValue().getValue();
if (!promise.awaitUninterruptibly(timeout, unit)) {
throw new IllegalStateException("Timed out waiting for response on stream id " + entry.getKey());
}
if (!promise.isSuccess()) {
if (promise.isFailed()) {
throw new RuntimeException(promise.cause());
}
System.out.println("---Stream id: " + entry.getKey() + " received---");

View File

@ -78,7 +78,7 @@ public class RedisClient {
// Sends the received line to the server.
lastWriteFuture = ch.writeAndFlush(line);
lastWriteFuture.addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
System.err.print("write failed: ");
future.cause().printStackTrace(System.err);
}

View File

@ -62,7 +62,7 @@ public abstract class ProxyHandler implements ChannelHandler {
private final Promise<Channel> connectPromise = new LazyPromise();
private ScheduledFuture<?> connectTimeoutFuture;
private final FutureListener<Void> writeListener = future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
setConnectFailure(future.cause());
}
};

View File

@ -169,7 +169,7 @@ abstract class ProxyServer {
this.finished = true;
Future<Channel> f = connectToDestination(ctx.channel().executor(), new BackendHandler(ctx));
f.addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
recordException(future.cause());
ctx.close();
} else {

View File

@ -100,7 +100,7 @@ public class SniHandler extends AbstractSniHandler<SslContext> {
@Override
protected final void onLookupComplete(ChannelHandlerContext ctx,
String hostname, Future<? extends SslContext> future) throws Exception {
if (!future.isSuccess()) {
if (future.isFailed()) {
final Throwable cause = future.cause();
if (cause instanceof Error) {
throw (Error) cause;

View File

@ -300,7 +300,7 @@ public class ChunkedWriteHandler implements ChannelHandler {
private static void handleEndOfInputFuture(Future<?> future, PendingWrite currentWrite) {
ChunkedInput<?> input = (ChunkedInput<?>) currentWrite.msg;
if (!future.isSuccess()) {
if (future.isFailed()) {
closeInput(input);
currentWrite.fail(future.cause());
} else {
@ -314,7 +314,7 @@ public class ChunkedWriteHandler implements ChannelHandler {
private void handleFuture(Channel channel, Future<?> future, PendingWrite currentWrite, boolean resume) {
ChunkedInput<?> input = (ChunkedInput<?>) currentWrite.msg;
if (!future.isSuccess()) {
if (future.isFailed()) {
closeInput(input);
currentWrite.fail(future.cause());
} else {

View File

@ -433,7 +433,7 @@ public class ParameterizedSslHandlerTest {
PromiseNotifier.cascade(handler.sslCloseFuture(), serverPromise);
handler.handshakeFuture().addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
// Something bad happened during handshake fail the promise!
serverPromise.tryFailure(future.cause());
}

View File

@ -81,7 +81,7 @@ public abstract class RenegotiateTest {
renegotiate = true;
handler.renegotiate().addListener((FutureListener<Channel>) future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
error.compareAndSet(null, future.cause());
ctx.close();
}

View File

@ -1276,7 +1276,7 @@ public class DnsNameResolver extends InetNameResolver {
.channelFactory(socketChannelFactory)
.handler(TCP_ENCODER);
bs.connect(res.sender()).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
if (logger.isDebugEnabled()) {
logger.debug("{} Unable to fallback to TCP [{}]", queryId, future.cause());
}

View File

@ -147,7 +147,7 @@ abstract class DnsQueryContext implements FutureListener<AddressedEnvelope<DnsRe
}
private void onQueryWriteCompletion(Future<? extends Void> writeFuture, Promise<Void> writePromise) {
if (!writeFuture.isSuccess()) {
if (writeFuture.isFailed()) {
writePromise.setFailure(writeFuture.cause());
tryFailure("failed to send a query via " + protocol(), writeFuture.cause(), false);
return;

View File

@ -91,7 +91,7 @@ public abstract class AbstractSocketReuseFdTest extends AbstractSocketTest {
});
FutureListener<Channel> listener = future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
clientDonePromise.tryFailure(future.cause());
}
};

View File

@ -65,7 +65,7 @@ public class EpollDomainSocketFdTest extends AbstractSocketTest {
final EpollDomainSocketChannel ch = new EpollDomainSocketChannel(ctx.channel().executor());
ctx.writeAndFlush(ch.fd()).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
Throwable cause = future.cause();
queue.offer(cause);
}

View File

@ -65,7 +65,7 @@ public class KQueueDomainSocketFdTest extends AbstractSocketTest {
final KQueueDomainSocketChannel ch = new KQueueDomainSocketChannel(ctx.channel().executor());
ctx.writeAndFlush(ch.fd()).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
Throwable cause = future.cause();
queue.offer(cause);
}

View File

@ -301,7 +301,7 @@ public abstract class AbstractBootstrap<B extends AbstractBootstrap<B, C, F>, C
Channel channel = newChannel(loop);
init(channel).addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
channel.unsafe().closeForcibly();
}
});

View File

@ -190,7 +190,7 @@ public class Bootstrap extends AbstractBootstrap<Bootstrap, Channel, ChannelFact
Promise<Channel> resolveAndConnectPromise = new DefaultPromise<>(loop);
if (regFuture.isDone()) {
if (!regFuture.isSuccess()) {
if (regFuture.isFailed()) {
return regFuture;
}
Channel channel = regFuture.getNow();

View File

@ -263,7 +263,7 @@ public class ServerBootstrap extends AbstractBootstrap<ServerBootstrap, ServerCh
child.pipeline().addLast(childHandler);
child.register().addListener(future -> {
if (!future.isSuccess()) {
if (future.isFailed()) {
forceClose(child, future.cause());
}
});

View File

@ -49,12 +49,12 @@ public enum ChannelFutureListeners implements FutureContextListener<Channel, Obj
channel.close();
break;
case CLOSE_ON_FAILURE:
if (!future.isSuccess()) {
if (future.isFailed()) {
channel.close();
}
break;
case FIRE_EXCEPTION_ON_FAILURE:
if (!future.isSuccess()) {
if (future.isFailed()) {
channel.pipeline().fireExceptionCaught(future.cause());
}
}

View File

@ -586,7 +586,7 @@ public class EmbeddedChannel extends AbstractChannel {
}
private void recordException(Future<?> future) {
if (!future.isSuccess()) {
if (future.isFailed()) {
recordException(future.cause());
}
}

View File

@ -62,7 +62,7 @@ final class DefaultChannelGroupFuture extends DefaultPromise<Void> implements Ch
List<Map.Entry<Channel, Throwable>> failed =
new ArrayList<>(failureCount);
for (Entry<Channel, Future<Void>> entry: futures.entrySet()) {
if (!entry.getValue().isSuccess()) {
if (entry.getValue().isFailed()) {
failed.add(new DefaultEntry<>(entry.getKey(), entry.getValue().cause()));
}
}