From 1e87c711b4404299fe140eabffa9be3e2ada6565 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 26 Jan 2021 19:26:20 +0100 Subject: [PATCH] Ensure we create a copy of the attributes and options when bootstrap a channel (#10965) Motivation: We need to ensure we copy the attributes and options when bootstrap the channel as otherwise we may change the underlying Entry. This is similar to what was reported in https://github.com/netty/netty-incubator-codec-quic/issues/152. Modifications: - Do a copy and re-use methods - Add unit tests Result: Don't affect attributes / options of channels that are already bootstrapped --- .../io/netty/bootstrap/AbstractBootstrap.java | 18 ++++++++++--- .../java/io/netty/bootstrap/Bootstrap.java | 2 +- .../io/netty/bootstrap/ServerBootstrap.java | 9 +++---- .../io/netty/bootstrap/BootstrapTest.java | 25 +++++++++++++++++++ 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java index 728e6366f0..addef88256 100644 --- a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java @@ -52,9 +52,9 @@ import java.util.concurrent.ConcurrentHashMap; */ public abstract class AbstractBootstrap, C extends Channel> implements Cloneable { @SuppressWarnings("unchecked") - static final Map.Entry, Object>[] EMPTY_OPTION_ARRAY = new Map.Entry[0]; + private static final Map.Entry, Object>[] EMPTY_OPTION_ARRAY = new Map.Entry[0]; @SuppressWarnings("unchecked") - static final Map.Entry, Object>[] EMPTY_ATTRIBUTE_ARRAY = new Map.Entry[0]; + private static final Map.Entry, Object>[] EMPTY_ATTRIBUTE_ARRAY = new Map.Entry[0]; volatile EventLoopGroup group; @SuppressWarnings("deprecation") @@ -386,11 +386,23 @@ public abstract class AbstractBootstrap, C ext public abstract AbstractBootstrapConfig config(); final Map.Entry, Object>[] newOptionsArray() { + return newOptionsArray(options); + } + + static Map.Entry, Object>[] newOptionsArray(Map, Object> options) { synchronized (options) { - return options.entrySet().toArray(EMPTY_OPTION_ARRAY); + return new LinkedHashMap, Object>(options).entrySet().toArray(EMPTY_OPTION_ARRAY); } } + final Map.Entry, Object>[] newAttributesArray() { + return newAttributesArray(attrs0()); + } + + static Map.Entry, Object>[] newAttributesArray(Map, Object> attributes) { + return attributes.entrySet().toArray(EMPTY_ATTRIBUTE_ARRAY); + } + final Map, Object> options0() { return options; } diff --git a/transport/src/main/java/io/netty/bootstrap/Bootstrap.java b/transport/src/main/java/io/netty/bootstrap/Bootstrap.java index 35e06b17c8..dbb6942f6b 100644 --- a/transport/src/main/java/io/netty/bootstrap/Bootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/Bootstrap.java @@ -263,7 +263,7 @@ public class Bootstrap extends AbstractBootstrap { p.addLast(config.handler()); setChannelOptions(channel, newOptionsArray(), logger); - setAttributes(channel, attrs0().entrySet().toArray(EMPTY_ATTRIBUTE_ARRAY)); + setAttributes(channel, newAttributesArray()); } @Override diff --git a/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java b/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java index 09e13b81c4..ce21a897bf 100644 --- a/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java +++ b/transport/src/main/java/io/netty/bootstrap/ServerBootstrap.java @@ -130,17 +130,14 @@ public class ServerBootstrap extends AbstractBootstrap, Object>[] currentChildOptions; - synchronized (childOptions) { - currentChildOptions = childOptions.entrySet().toArray(EMPTY_OPTION_ARRAY); - } - final Entry, Object>[] currentChildAttrs = childAttrs.entrySet().toArray(EMPTY_ATTRIBUTE_ARRAY); + final Entry, Object>[] currentChildOptions = newOptionsArray(childOptions); + final Entry, Object>[] currentChildAttrs = newAttributesArray(childAttrs); p.addLast(new ChannelInitializer() { @Override diff --git a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java index 254630a7d8..05b3da299e 100644 --- a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java +++ b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java @@ -38,6 +38,7 @@ import io.netty.channel.local.LocalServerChannel; import io.netty.resolver.AddressResolver; import io.netty.resolver.AddressResolverGroup; import io.netty.resolver.AbstractAddressResolver; +import io.netty.util.AttributeKey; import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Future; import io.netty.util.concurrent.Promise; @@ -51,6 +52,8 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Map; +import java.util.UUID; import java.util.concurrent.BlockingQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.LinkedBlockingQueue; @@ -73,6 +76,28 @@ public class BootstrapTest { groupB.terminationFuture().syncUninterruptibly(); } + @Test + public void testOptionsCopied() { + final Bootstrap bootstrapA = new Bootstrap(); + bootstrapA.option(ChannelOption.AUTO_READ, true); + Map.Entry, Object>[] channelOptions = bootstrapA.newOptionsArray(); + bootstrapA.option(ChannelOption.AUTO_READ, false); + assertEquals(ChannelOption.AUTO_READ, channelOptions[0].getKey()); + assertEquals(true, channelOptions[0].getValue()); + } + + @Test + public void testAttributesCopied() { + AttributeKey key = AttributeKey.valueOf(UUID.randomUUID().toString()); + String value = "value"; + final Bootstrap bootstrapA = new Bootstrap(); + bootstrapA.attr(key, value); + Map.Entry, Object>[] attributesArray = bootstrapA.newAttributesArray(); + bootstrapA.attr(key, "value2"); + assertEquals(key, attributesArray[0].getKey()); + assertEquals(value, attributesArray[0].getValue()); + } + @Test(timeout = 10000) public void testBindDeadLock() throws Exception { final Bootstrap bootstrapA = new Bootstrap();