From 56055f4404fbebeb6403165d352d62330a1bf81d Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 6 Feb 2020 09:02:31 +0100 Subject: [PATCH] Ensure ChannelOptions are applied in the same order as configured in *Bootstrap (#9998) Motivation: https://github.com/netty/netty/pull/9458 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them. Modifications: - Use again a LinkedHashMap to preserve order - Add unit test Result: Apply ChannelOptions in correct and expected order --- .../io/netty/bootstrap/AbstractBootstrap.java | 30 +++++++--- .../java/io/netty/bootstrap/Bootstrap.java | 2 +- .../io/netty/bootstrap/ServerBootstrap.java | 31 ++++++---- .../io/netty/bootstrap/BootstrapTest.java | 57 +++++++++++++++++++ 4 files changed, 102 insertions(+), 18 deletions(-) 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) {