diff --git a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java index 50b929a3f7..cd3e741936 100644 --- a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java @@ -39,6 +39,7 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -59,7 +60,10 @@ public abstract class AbstractBootstrap, C ext @SuppressWarnings("deprecation") private volatile ChannelFactory channelFactory; private volatile SocketAddress localAddress; - private final Map, Object> options = new ConcurrentHashMap, Object>(); + + // The order in which ChannelOptions are applied is important they may depend on each other for validation + // purposes. + private final Map, Object> options = new LinkedHashMap, Object>(); private final Map, Object> attrs = new ConcurrentHashMap, Object>(); private volatile ChannelHandler handler; @@ -72,7 +76,9 @@ public abstract class AbstractBootstrap, C ext channelFactory = bootstrap.channelFactory; handler = bootstrap.handler; localAddress = bootstrap.localAddress; - options.putAll(bootstrap.options); + synchronized (bootstrap.options) { + options.putAll(bootstrap.options); + } attrs.putAll(bootstrap.attrs); } @@ -166,10 +172,12 @@ public abstract class AbstractBootstrap, C ext */ public B option(ChannelOption option, T value) { ObjectUtil.checkNotNull(option, "option"); - if (value == null) { - options.remove(option); - } else { - options.put(option, value); + synchronized (options) { + if (value == null) { + options.remove(option); + } else { + options.put(option, value); + } } return self(); } @@ -377,6 +385,12 @@ public abstract class AbstractBootstrap, C ext */ public abstract AbstractBootstrapConfig config(); + final Map.Entry, Object>[] newOptionsArray() { + synchronized (options) { + return options.entrySet().toArray(EMPTY_OPTION_ARRAY); + } + } + final Map, Object> options0() { return options; } @@ -399,7 +413,9 @@ public abstract class AbstractBootstrap, C ext } final Map, Object> options() { - return copiedMap(options); + synchronized (options) { + return copiedMap(options); + } } final Map, Object> attrs() { diff --git a/transport/src/main/java/io/netty/bootstrap/Bootstrap.java b/transport/src/main/java/io/netty/bootstrap/Bootstrap.java index 3822b43b0f..28e80ceae2 100644 --- a/transport/src/main/java/io/netty/bootstrap/Bootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/Bootstrap.java @@ -256,7 +256,7 @@ public class Bootstrap extends AbstractBootstrap { ChannelPipeline p = channel.pipeline(); p.addLast(config.handler()); - setChannelOptions(channel, options0().entrySet().toArray(EMPTY_OPTION_ARRAY), logger); + setChannelOptions(channel, newOptionsArray(), logger); setAttributes(channel, attrs0().entrySet().toArray(EMPTY_ATTRIBUTE_ARRAY)); } diff --git a/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java b/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java index 17815790cb..37dd350788 100644 --- a/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java @@ -32,6 +32,7 @@ import io.netty.util.internal.ObjectUtil; import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLoggerFactory; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.ConcurrentHashMap; @@ -45,7 +46,9 @@ public class ServerBootstrap extends AbstractBootstrap, Object> childOptions = new ConcurrentHashMap, Object>(); + // The order in which child ChannelOptions are applied is important they may depend on each other for validation + // purposes. + private final Map, Object> childOptions = new LinkedHashMap, Object>(); private final Map, Object> childAttrs = new ConcurrentHashMap, Object>(); private final ServerBootstrapConfig config = new ServerBootstrapConfig(this); private volatile EventLoopGroup childGroup; @@ -57,7 +60,9 @@ public class ServerBootstrap extends AbstractBootstrap ServerBootstrap childOption(ChannelOption childOption, T value) { ObjectUtil.checkNotNull(childOption, "childOption"); - if (value == null) { - childOptions.remove(childOption); - } else { - childOptions.put(childOption, value); + synchronized (childOptions) { + if (value == null) { + childOptions.remove(childOption); + } else { + childOptions.put(childOption, value); + } } return this; } @@ -122,15 +129,17 @@ public class ServerBootstrap extends AbstractBootstrap, Object>[] currentChildOptions = - childOptions.entrySet().toArray(EMPTY_OPTION_ARRAY); + final Entry, Object>[] currentChildOptions; + synchronized (childOptions) { + currentChildOptions = childOptions.entrySet().toArray(EMPTY_OPTION_ARRAY); + } final Entry, Object>[] currentChildAttrs = childAttrs.entrySet().toArray(EMPTY_ATTRIBUTE_ARRAY); p.addLast(new ChannelInitializer() { @@ -261,7 +270,9 @@ public class ServerBootstrap extends AbstractBootstrap, Object> childOptions() { - return copiedMap(childOptions); + synchronized (childOptions) { + return copiedMap(childOptions); + } } final Map, Object> childAttrs() { diff --git a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java index cc93a91db2..b3744433f4 100644 --- a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java +++ b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java @@ -17,15 +17,20 @@ package io.netty.bootstrap; import io.netty.channel.Channel; +import io.netty.channel.ChannelConfig; import io.netty.channel.ChannelFactory; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandler.Sharable; import io.netty.channel.ChannelInboundHandler; import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.ChannelOption; import io.netty.channel.ChannelPromise; +import io.netty.channel.DefaultChannelConfig; import io.netty.channel.DefaultEventLoop; import io.netty.channel.DefaultEventLoopGroup; +import io.netty.channel.EventLoop; import io.netty.channel.EventLoopGroup; import io.netty.channel.ServerChannel; import io.netty.channel.local.LocalAddress; @@ -47,7 +52,9 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Queue; import java.util.concurrent.BlockingQueue; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.LinkedBlockingQueue; import static org.hamcrest.Matchers.*; @@ -293,6 +300,56 @@ public class BootstrapTest { assertThat(connectFuture.channel(), is(not(nullValue()))); } + @Test + public void testChannelOptionOrderPreserve() throws InterruptedException { + final BlockingQueue> options = new LinkedBlockingQueue>(); + class ChannelConfigValidator extends DefaultChannelConfig { + ChannelConfigValidator(Channel channel) { + super(channel); + } + + @Override + public boolean setOption(ChannelOption option, T value) { + options.add(option); + return super.setOption(option, value); + } + } + final CountDownLatch latch = new CountDownLatch(1); + final Bootstrap bootstrap = new Bootstrap() + .handler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) { + latch.countDown(); + } + }) + .group(groupA) + .channelFactory(new ChannelFactory() { + @Override + public Channel newChannel() { + return new LocalChannel() { + private ChannelConfigValidator config; + @Override + public synchronized ChannelConfig config() { + if (config == null) { + config = new ChannelConfigValidator(this); + } + return config; + } + }; + } + }) + .option(ChannelOption.WRITE_BUFFER_LOW_WATER_MARK, 1) + .option(ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK, 2); + + bootstrap.register().syncUninterruptibly(); + + latch.await(); + + // Check the order is the same as what we defined before. + assertSame(ChannelOption.WRITE_BUFFER_LOW_WATER_MARK, options.take()); + assertSame(ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK, options.take()); + } + private static final class DelayedEventLoopGroup extends DefaultEventLoop { @Override public ChannelFuture register(final Channel channel, final ChannelPromise promise) {