Skip to content

Bump Netty to 4.2.2 #6205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bom-internal/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-codec</artifactId>
<artifactId>netty-codec-base</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did Netty deprecate netty-codec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The netty-codec module has been split into a number of different sub-modules, which the netty-codec module then depends on. In other words, netty-codec is now multiple jar files instead of one.

<version>${netty.version}</version>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion bundle-sdk/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
</exclusion>
<exclusion>
<groupId>io.netty</groupId>
<artifactId>netty-codec</artifactId>
<artifactId>netty-codec-base</artifactId>
</exclusion>
<exclusion>
<groupId>io.netty</groupId>
Expand Down
2 changes: 1 addition & 1 deletion http-clients/netty-nio-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-codec</artifactId>
<artifactId>netty-codec-base</artifactId>
</dependency>
<dependency>
<groupId>io.netty</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import io.netty.channel.Channel;
import io.netty.channel.ChannelFactory;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.IoHandlerFactory;
import io.netty.channel.MultiThreadIoEventLoopGroup;
import io.netty.channel.nio.NioIoHandler;
import io.netty.channel.socket.DatagramChannel;
import io.netty.channel.socket.nio.NioDatagramChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
Expand All @@ -32,7 +34,7 @@
/**
* Provides {@link EventLoopGroup} and {@link ChannelFactory} for {@link NettyNioAsyncHttpClient}.
* <p>
* There are three ways to create a new instance.
* There are four ways to create a new instance.
*
* <ul>
* <li>using {@link #builder()} to provide custom configuration of {@link EventLoopGroup}.
Expand All @@ -47,6 +49,9 @@
*
* <li>Using {@link #create(EventLoopGroup, ChannelFactory)} to provide a custom {@link EventLoopGroup} and
* {@link ChannelFactory}
*
* <li>Using {@link #create(EventLoopGroup, ChannelFactory, ChannelFactory)} to provide a custom {@link EventLoopGroup}, and
* socket and datagram {@link ChannelFactory}'s.
* </ul>
*
* <p>
Expand Down Expand Up @@ -76,6 +81,16 @@ public final class SdkEventLoopGroup {
this.datagramChannelFactory = ChannelResolver.resolveDatagramChannelFactory(eventLoopGroup);
}

SdkEventLoopGroup(EventLoopGroup eventLoopGroup, ChannelFactory<? extends Channel> socketChannelFactory,
ChannelFactory<? extends DatagramChannel> datagramChannelFactory) {
Validate.paramNotNull(eventLoopGroup, "eventLoopGroup");
Validate.paramNotNull(socketChannelFactory, "socketChannelFactory");
Validate.paramNotNull(datagramChannelFactory, "datagramChannelFactory");
this.eventLoopGroup = eventLoopGroup;
this.channelFactory = socketChannelFactory;
this.datagramChannelFactory = datagramChannelFactory;
}

/**
* Create an instance of {@link SdkEventLoopGroup} from the builder
*/
Expand Down Expand Up @@ -107,15 +122,30 @@ public ChannelFactory<? extends DatagramChannel> datagramChannelFactory() {
}

/**
* Creates a new instance of SdkEventLoopGroup with {@link EventLoopGroup} and {@link ChannelFactory}
* Creates a new instance of SdkEventLoopGroup with {@link EventLoopGroup} and socket {@link ChannelFactory}
* to be used with {@link NettyNioAsyncHttpClient}.
*
* @param eventLoopGroup the EventLoopGroup to be used
* @param channelFactory the channel factor to be used
* @param socketChannelFactory the socket channel factory to be used
* @return a new instance of SdkEventLoopGroup
*/
public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup,
ChannelFactory<? extends Channel> socketChannelFactory) {
return new SdkEventLoopGroup(eventLoopGroup, socketChannelFactory);
}

/**
* Creates a new instance of SdkEventLoopGroup with {@link EventLoopGroup}, and socket and datagram
* {@link ChannelFactory}'s to be used with {@link NettyNioAsyncHttpClient}.
*
* @param eventLoopGroup the EventLoopGroup to be used
* @param socketChannelFactory the socket channel factory to be used
* @param datagramChannelFactory the datagram channel factory to be used
* @return a new instance of SdkEventLoopGroup
*/
public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup, ChannelFactory<? extends Channel> channelFactory) {
return new SdkEventLoopGroup(eventLoopGroup, channelFactory);
public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup, ChannelFactory<? extends Channel> socketChannelFactory,
ChannelFactory<? extends DatagramChannel> datagramChannelFactory) {
return new SdkEventLoopGroup(eventLoopGroup, socketChannelFactory, datagramChannelFactory);
}

/**
Expand All @@ -124,6 +154,13 @@ public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup, ChannelFac
* <p>
* {@link ChannelFactory} will be resolved based on the type of {@link EventLoopGroup} provided. IllegalArgumentException will
* be thrown for any unknown EventLoopGroup type.
* <p>
* If {@link MultiThreadIoEventLoopGroup} is passed in, {@link NioSocketChannel} and {@link NioDatagramChannel} will be
* resolved, regardless of the transport {@link IoHandlerFactory} set on the {@link MultiThreadIoEventLoopGroup}. This is
* because it is not possible to determine the type of transport factory from a given {@link MultiThreadIoEventLoopGroup}.
* <p>
* To use a {@link MultiThreadIoEventLoopGroup} with a non-Nio transport factory, use
* {@link #create(EventLoopGroup, ChannelFactory, ChannelFactory)}, specifying the socket and datagram channels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is worded is hard to follow. From what I understand, if the customer calls this with a MultiThreadIoEventLoopGroup (and not one of its subclasses like NioEventLoopGroup), then the SDK will disregard the IoHandler the event loop group was constructed with when choosing the factory, and always assume NioSocketChannel::new.

And if they want to ensure that specific factories are used, to use one of the other overloads?

What happens if there is a mismatch in the IoHandlerFactory and the resolved SocketFactory (e.g. epoll vs NIO)? Is there a clear error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if they want to ensure that specific factories are used, to use one of the other overloads?

Correct, I'll try to make the wording more clear

What happens if there is a mismatch in the IoHandlerFactory and the resolved SocketFactory (e.g. epoll vs NIO)? Is there a clear error message?

Tested this:

        SdkEventLoopGroup sdkEventLoopGroup =
            SdkEventLoopGroup.create(new MultiThreadIoEventLoopGroup(KQueueIoHandler.newFactory()),
                                     NioSocketChannel::new,
                                     NioDatagramChannel::new);

        SdkAsyncHttpClient netty = NettyNioAsyncHttpClient.builder()
                                                          .eventLoopGroup(sdkEventLoopGroup)
                                                          .build();
        S3AsyncClient asyncClient = S3AsyncClient.builder()
                                                 .region(DEFAULT_REGION)
                                                 .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN)
                                                 .httpClient(netty)

                                                 .build();
        asyncClient.headObject(c -> c.bucket(BUCKET).key(ASYNC_KEY)).join();

Got error message:

java.util.concurrent.CompletionException: software.amazon.awssdk.core.exception.SdkClientException: Unable to execute HTTP request: incompatible event loop type: io.netty.channel.SingleThreadIoEventLoop (SDK Attempt Count: 1)
...
Caused by: java.lang.IllegalStateException: incompatible event loop type: io.netty.channel.SingleThreadIoEventLoop
	at io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:332)
	at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:119)
	at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:113)
	at io.netty.channel.MultithreadEventLoopGroup.register(MultithreadEventLoopGroup.java:86)
	at software.amazon.awssdk.http.nio.netty.internal.DelegatingEventLoopGroup.register(DelegatingEventLoopGroup.java:173)
	at io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:339)
	at io.netty.bootstrap.Bootstrap.doResolveAndConnect(Bootstrap.java:164)
	at io.netty.bootstrap.Bootstrap.connect(Bootstrap.java:125)

*
* @param eventLoopGroup the EventLoopGroup to be used
* @return a new instance of SdkEventLoopGroup
Expand All @@ -142,7 +179,7 @@ private EventLoopGroup resolveEventLoopGroup(DefaultBuilder builder) {
.orElseGet(() -> new ThreadFactoryBuilder()
.threadNamePrefix("aws-java-sdk-NettyEventLoop")
.build());
return new NioEventLoopGroup(numThreads, threadFactory);
return new MultiThreadIoEventLoopGroup(numThreads, threadFactory, NioIoHandler.newFactory());
/*
Need to investigate why epoll is raising channel inactive after successful response that causes
problems with retries.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.netty.channel.Channel;
import io.netty.channel.ChannelFactory;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.MultiThreadIoEventLoopGroup;
import io.netty.channel.ReflectiveChannelFactory;
import io.netty.channel.epoll.EpollDatagramChannel;
import io.netty.channel.epoll.EpollEventLoopGroup;
Expand Down Expand Up @@ -75,11 +76,17 @@ public static ChannelFactory<? extends Channel> resolveSocketChannelFactory(Even
}

String socketFqcn = KNOWN_EL_GROUPS_SOCKET_CHANNELS.get(eventLoopGroup.getClass().getName());
if (socketFqcn == null) {
throw new IllegalArgumentException("Unknown event loop group : " + eventLoopGroup.getClass());
if (socketFqcn != null) {
return invokeSafely(() -> new ReflectiveChannelFactory(Class.forName(socketFqcn)));
}

return invokeSafely(() -> new ReflectiveChannelFactory(Class.forName(socketFqcn)));
// The old deprecated transport-specific event loop groups all extend MultiThreadIoEventLoopGroup, so we have to do
// this check last. It is not possible to determine the type of transport factory so we will default to Nio.
if (eventLoopGroup instanceof MultiThreadIoEventLoopGroup) {
return NioSocketChannel::new;
}

throw new IllegalArgumentException("Unknown event loop group : " + eventLoopGroup.getClass());
}

/**
Expand All @@ -103,10 +110,16 @@ public static ChannelFactory<? extends DatagramChannel> resolveDatagramChannelFa
}

String datagramFqcn = KNOWN_EL_GROUPS_DATAGRAM_CHANNELS.get(eventLoopGroup.getClass().getName());
if (datagramFqcn == null) {
throw new IllegalArgumentException("Unknown event loop group : " + eventLoopGroup.getClass());
if (datagramFqcn != null) {
return invokeSafely(() -> new ReflectiveChannelFactory(Class.forName(datagramFqcn)));
}

// The old deprecated transport-specific event loop groups all extend MultiThreadIoEventLoopGroup, so we have to do
// this check last. It is not possible to determine the type of transport factory so we will default to Nio.
if (eventLoopGroup instanceof MultiThreadIoEventLoopGroup) {
return NioDatagramChannel::new;
}

return invokeSafely(() -> new ReflectiveChannelFactory(Class.forName(datagramFqcn)));
throw new IllegalArgumentException("Unknown event loop group : " + eventLoopGroup.getClass());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,6 @@ public static SslHandler newSslHandler(SslContext sslContext, ByteBufAllocator a
*/
private static void configureSslEngine(SSLEngine sslEngine) {
SSLParameters sslParameters = sslEngine.getSSLParameters();
sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sslEngine.setSSLParameters(sslParameters);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,23 @@
package software.amazon.awssdk.http.nio.netty;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import io.netty.channel.DefaultEventLoopGroup;
import io.netty.channel.epoll.Epoll;
import io.netty.channel.epoll.EpollDatagramChannel;
import io.netty.channel.epoll.EpollEventLoopGroup;
import io.netty.channel.epoll.EpollIoHandler;
import io.netty.channel.epoll.EpollSocketChannel;
import io.netty.channel.MultiThreadIoEventLoopGroup;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.nio.NioIoHandler;
import io.netty.channel.oio.OioEventLoopGroup;
import io.netty.channel.socket.nio.NioDatagramChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
import io.netty.channel.socket.oio.OioDatagramChannel;
import io.netty.channel.socket.oio.OioSocketChannel;
import org.junit.Test;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;

public class SdkEventLoopGroupTest {

Expand Down Expand Up @@ -64,8 +69,42 @@ public void notProvidingChannelFactory_channelFactoryResolved() {
assertThat(sdkEventLoopGroup.datagramChannelFactory().newChannel()).isInstanceOf(NioDatagramChannel.class);
}

@Test(expected = IllegalArgumentException.class)
@Test
public void multiThreadIoEventLoopGroup_nioIoHandler_withoutChannelFactory() {
SdkEventLoopGroup sdkEventLoopGroup =
SdkEventLoopGroup.create(new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory()));

assertThat(sdkEventLoopGroup.channelFactory().newChannel()).isInstanceOf(NioSocketChannel.class);
assertThat(sdkEventLoopGroup.datagramChannelFactory().newChannel()).isInstanceOf(NioDatagramChannel.class);
}

@Test
public void multiThreadIoEventLoopGroupWithNonNioIoHandler_withoutChannelFactory_createsNioChannels() {
Assumptions.assumeTrue(Epoll.isAvailable());

SdkEventLoopGroup sdkEventLoopGroup =
SdkEventLoopGroup.create(new MultiThreadIoEventLoopGroup(EpollIoHandler.newFactory()));

assertThat(sdkEventLoopGroup.channelFactory().newChannel()).isInstanceOf(NioSocketChannel.class);
assertThat(sdkEventLoopGroup.datagramChannelFactory().newChannel()).isInstanceOf(NioDatagramChannel.class);
}

@Test
public void multiThreadIoEventLoopGroupWithNonNioIoHandler_withChannelsFactories_properlySetsChannelTypes() {
Assumptions.assumeTrue(Epoll.isAvailable());

SdkEventLoopGroup sdkEventLoopGroup =
SdkEventLoopGroup.create(new MultiThreadIoEventLoopGroup(EpollIoHandler.newFactory()),
EpollSocketChannel::new,
EpollDatagramChannel::new);
assertThat(sdkEventLoopGroup.channelFactory()).isNotNull();
assertThat(sdkEventLoopGroup.channelFactory().newChannel()).isInstanceOf(EpollSocketChannel.class);
assertThat(sdkEventLoopGroup.datagramChannelFactory().newChannel()).isInstanceOf(EpollDatagramChannel.class);
assertThat(sdkEventLoopGroup.eventLoopGroup()).isNotNull();
}

@Test
public void notProvidingChannelFactory_unknownEventLoopGroup() {
SdkEventLoopGroup.create(new DefaultEventLoopGroup());
assertThrows(IllegalArgumentException.class, () -> SdkEventLoopGroup.create(new DefaultEventLoopGroup()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelInitializer;
import io.netty.channel.ChannelPipeline;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.MultiThreadIoEventLoopGroup;
import io.netty.channel.SimpleChannelInboundHandler;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.nio.NioIoHandler;
import io.netty.channel.socket.ServerSocketChannel;
import io.netty.channel.socket.nio.NioServerSocketChannel;
import io.netty.handler.codec.http.DefaultHttpContent;
Expand Down Expand Up @@ -312,7 +314,7 @@ public RequestParams build(){
}

private static class Server extends ChannelInitializer<Channel> {
private final NioEventLoopGroup group = new NioEventLoopGroup();
private final EventLoopGroup group = new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory());
private CloseTime closeTime;
private ServerBootstrap bootstrap;
private ServerSocketChannel serverSock;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
import static software.amazon.awssdk.http.nio.netty.internal.utils.ChannelResolver.resolveDatagramChannelFactory;
import static software.amazon.awssdk.http.nio.netty.internal.utils.ChannelResolver.resolveSocketChannelFactory;

import io.netty.channel.MultiThreadIoEventLoopGroup;
import io.netty.channel.epoll.Epoll;
import io.netty.channel.epoll.EpollDatagramChannel;
import io.netty.channel.epoll.EpollEventLoopGroup;
import io.netty.channel.epoll.EpollSocketChannel;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.nio.NioIoHandler;
import io.netty.channel.oio.OioEventLoopGroup;
import io.netty.channel.socket.nio.NioDatagramChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
Expand All @@ -41,6 +43,12 @@ public void canDetectFactoryForStandardNioEventLoopGroup() {
assertThat(resolveDatagramChannelFactory(new NioEventLoopGroup()).newChannel()).isInstanceOf(NioDatagramChannel.class);
}

@Test
public void multiThreadIoEventLoopGroup_returnsNioChannels() {
assertThat(resolveSocketChannelFactory(new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory())).newChannel()).isInstanceOf(NioSocketChannel.class);
assertThat(resolveDatagramChannelFactory(new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory())).newChannel()).isInstanceOf(NioDatagramChannel.class);
}

@Test
public void canDetectEpollEventLoopGroupFactory() {
Assumptions.assumeTrue(Epoll.isAvailable());
Expand Down
4 changes: 2 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@
<equalsverifier.version>3.15.1</equalsverifier.version>
<!-- Update netty-open-ssl-version accordingly whenever we update netty version-->
<!-- https://github.com/netty/netty/blob/4.1/pom.xml search "tcnative.version" -->
<netty.version>4.1.118.Final</netty.version>
<netty.version>4.2.2.Final</netty.version>
<unitils.version>3.4.6</unitils.version>
<xmlunit.version>1.3</xmlunit.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
Expand All @@ -139,7 +139,7 @@
<jimfs.version>1.1</jimfs.version>
<testng.version>7.1.0</testng.version> <!-- TCK Tests -->
<commons-lang.verson>2.6</commons-lang.verson>
<netty-open-ssl-version>2.0.70.Final</netty-open-ssl-version>
<netty-open-ssl-version>2.0.72.Final</netty-open-ssl-version>
<dynamodb-local.version>1.25.0</dynamodb-local.version>
<sqllite.version>1.0.392</sqllite.version>
<blockhound.version>1.0.8.RELEASE</blockhound.version>
Expand Down
4 changes: 2 additions & 2 deletions test/http-client-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>
<version>1.70</version>
<artifactId>bcpkix-jdk18on</artifactId>
<version>1.80</version>
<scope>compile</scope>
</dependency>
<dependency>
Expand Down
4 changes: 2 additions & 2 deletions test/protocol-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>
<version>1.70</version>
<artifactId>bcpkix-jdk18on</artifactId>
<version>1.80</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Loading