From 1740f366eb728ea5a0a63d18e9042161673414cd Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 22 Dec 2017 13:19:34 +0100 Subject: [PATCH] Fail fast when DefaultChannelPromise is constructed with null as Channel. Motivation: We should fail fast when DefaultChannelPromise is constructed with null as Channel as otherwise it will fail with a NPE once we call setSuccess / setFailure. Modifications: Add null check and test. Result: Fail fast. --- .../io/netty/bootstrap/AbstractBootstrap.java | 4 +- .../io/netty/bootstrap/FailedChannel.java | 107 ++++++++++++++++++ .../netty/channel/DefaultChannelPromise.java | 6 +- .../io/netty/bootstrap/BootstrapTest.java | 2 +- .../channel/DefaultChannelPromiseTest.java | 38 +++++++ 5 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 transport/src/main/java/io/netty/bootstrap/FailedChannel.java create mode 100644 transport/src/test/java/io/netty/channel/DefaultChannelPromiseTest.java diff --git a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java index db5211616f..fb05c8b263 100644 --- a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java @@ -323,9 +323,11 @@ public abstract class AbstractBootstrap, C ext if (channel != null) { // channel can be null if newChannel crashed (eg SocketException("too many open files")) channel.unsafe().closeForcibly(); + // as the Channel is not registered yet we need to force the usage of the GlobalEventExecutor + return new DefaultChannelPromise(channel, GlobalEventExecutor.INSTANCE).setFailure(t); } // as the Channel is not registered yet we need to force the usage of the GlobalEventExecutor - return new DefaultChannelPromise(channel, GlobalEventExecutor.INSTANCE).setFailure(t); + return new DefaultChannelPromise(new FailedChannel(), GlobalEventExecutor.INSTANCE).setFailure(t); } ChannelFuture regFuture = config().group().register(channel); diff --git a/transport/src/main/java/io/netty/bootstrap/FailedChannel.java b/transport/src/main/java/io/netty/bootstrap/FailedChannel.java new file mode 100644 index 0000000000..84c30fb651 --- /dev/null +++ b/transport/src/main/java/io/netty/bootstrap/FailedChannel.java @@ -0,0 +1,107 @@ +/* + * Copyright 2017 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.bootstrap; + +import io.netty.channel.AbstractChannel; +import io.netty.channel.ChannelConfig; +import io.netty.channel.ChannelMetadata; +import io.netty.channel.ChannelOutboundBuffer; +import io.netty.channel.ChannelPromise; +import io.netty.channel.DefaultChannelConfig; +import io.netty.channel.EventLoop; + +import java.net.SocketAddress; + +final class FailedChannel extends AbstractChannel { + private static final ChannelMetadata METADATA = new ChannelMetadata(false); + private final ChannelConfig config = new DefaultChannelConfig(this); + + FailedChannel() { + super(null); + } + + @Override + protected AbstractUnsafe newUnsafe() { + return new FailedChannelUnsafe(); + } + + @Override + protected boolean isCompatible(EventLoop loop) { + return false; + } + + @Override + protected SocketAddress localAddress0() { + return null; + } + + @Override + protected SocketAddress remoteAddress0() { + return null; + } + + @Override + protected void doBind(SocketAddress localAddress) { + throw new UnsupportedOperationException(); + } + + @Override + protected void doDisconnect() { + throw new UnsupportedOperationException(); + } + + @Override + protected void doClose() { + throw new UnsupportedOperationException(); + } + + @Override + protected void doBeginRead() { + throw new UnsupportedOperationException(); + } + + @Override + protected void doWrite(ChannelOutboundBuffer in) { + throw new UnsupportedOperationException(); + } + + @Override + public ChannelConfig config() { + return config; + } + + @Override + public boolean isOpen() { + return false; + } + + @Override + public boolean isActive() { + return false; + } + + @Override + public ChannelMetadata metadata() { + return METADATA; + } + + private final class FailedChannelUnsafe extends AbstractUnsafe { + @Override + public void connect(SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) { + promise.setFailure(new UnsupportedOperationException()); + } + } +} diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPromise.java b/transport/src/main/java/io/netty/channel/DefaultChannelPromise.java index 0fc89b9069..bb0bf3cd06 100644 --- a/transport/src/main/java/io/netty/channel/DefaultChannelPromise.java +++ b/transport/src/main/java/io/netty/channel/DefaultChannelPromise.java @@ -21,6 +21,8 @@ import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Future; import io.netty.util.concurrent.GenericFutureListener; +import static io.netty.util.internal.ObjectUtil.checkNotNull; + /** * The default {@link ChannelPromise} implementation. It is recommended to use {@link Channel#newPromise()} to create * a new {@link ChannelPromise} rather than calling the constructor explicitly. @@ -37,7 +39,7 @@ public class DefaultChannelPromise extends DefaultPromise implements Chann * the {@link Channel} associated with this future */ public DefaultChannelPromise(Channel channel) { - this.channel = channel; + this.channel = checkNotNull(channel, "channel"); } /** @@ -48,7 +50,7 @@ public class DefaultChannelPromise extends DefaultPromise implements Chann */ public DefaultChannelPromise(Channel channel, EventExecutor executor) { super(executor); - this.channel = channel; + this.channel = checkNotNull(channel, "channel"); } @Override diff --git a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java index db00b98414..cc93a91db2 100644 --- a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java +++ b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java @@ -290,7 +290,7 @@ public class BootstrapTest { // Should fail with the RuntimeException. assertThat(connectFuture.await(10000), is(true)); assertThat(connectFuture.cause(), sameInstance((Throwable) exception)); - assertThat(connectFuture.channel(), is(nullValue())); + assertThat(connectFuture.channel(), is(not(nullValue()))); } private static final class DelayedEventLoopGroup extends DefaultEventLoop { diff --git a/transport/src/test/java/io/netty/channel/DefaultChannelPromiseTest.java b/transport/src/test/java/io/netty/channel/DefaultChannelPromiseTest.java new file mode 100644 index 0000000000..ed2b5e4255 --- /dev/null +++ b/transport/src/test/java/io/netty/channel/DefaultChannelPromiseTest.java @@ -0,0 +1,38 @@ +/* + * Copyright 2017 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.channel; + +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.util.concurrent.ImmediateEventExecutor; +import org.junit.Test; + +public class DefaultChannelPromiseTest { + + @Test(expected = NullPointerException.class) + public void testNullChannel() { + new DefaultChannelPromise(null); + } + + @Test(expected = NullPointerException.class) + public void testChannelWithNullExecutor() { + new DefaultChannelPromise(new EmbeddedChannel(), null); + } + + @Test(expected = NullPointerException.class) + public void testNullChannelWithExecutor() { + new DefaultChannelPromise(null, ImmediateEventExecutor.INSTANCE); + } +}