-
Notifications
You must be signed in to change notification settings - Fork 1k
Now it's possible to configure NettyNioAsyncHttpClient for non blocking DNS #3990
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "category": "Netty NIO HTTP Client", | ||
| "contributor": "martinKindall", | ||
| "type": "bugfix", | ||
| "description": "By default, Netty threads are blocked during dns resolution, namely InetAddress.getByName is used under the hood. Now, there's an option to configure the NettyNioAsyncHttpClient in order to use a non blocking dns resolution strategy." | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,6 +103,7 @@ private NettyNioAsyncHttpClient(DefaultBuilder builder, AttributeMap serviceDefa | |
| .sdkEventLoopGroup(sdkEventLoopGroup) | ||
| .sslProvider(resolveSslProvider(builder)) | ||
| .proxyConfiguration(builder.proxyConfiguration) | ||
| .useNonBlockingDnsResolver(builder.useNonBlockingDnsResolver) | ||
| .build(); | ||
| } | ||
|
|
||
|
|
@@ -475,6 +476,15 @@ public interface Builder extends SdkAsyncHttpClient.Builder<NettyNioAsyncHttpCli | |
| * @return the builder for method chaining. | ||
| */ | ||
| Builder http2Configuration(Consumer<Http2Configuration.Builder> http2ConfigurationBuilderConsumer); | ||
|
|
||
| /** | ||
| * Configure whether to use a non-blocking dns resolver or not. False by default, as netty's default dns resolver is | ||
| * blocking; it namely calls java.net.InetAddress.getByName. | ||
| * <p> | ||
| * When enabled, a non-blocking dns resolver will be used instead, by modifying netty's bootstrap configuration. | ||
| * See https://netty.io/news/2016/05/26/4-1-0-Final.html | ||
| */ | ||
| Builder useNonBlockingDnsResolver(Boolean useNonBlockingDnsResolver); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -492,6 +502,7 @@ private static final class DefaultBuilder implements Builder { | |
| private Http2Configuration http2Configuration; | ||
| private SslProvider sslProvider; | ||
| private ProxyConfiguration proxyConfiguration; | ||
| private Boolean useNonBlockingDnsResolver; | ||
|
|
||
| private DefaultBuilder() { | ||
| } | ||
|
|
@@ -716,6 +727,16 @@ public void setHttp2Configuration(Http2Configuration http2Configuration) { | |
| http2Configuration(http2Configuration); | ||
| } | ||
|
|
||
| @Override | ||
| public Builder useNonBlockingDnsResolver(Boolean useNonBlockingDnsResolver) { | ||
| this.useNonBlockingDnsResolver = useNonBlockingDnsResolver; | ||
| return this; | ||
| } | ||
|
|
||
| public void setUseNonBlockingDnsResolver(Boolean useNonBlockingDnsResolver) { | ||
| useNonBlockingDnsResolver(useNonBlockingDnsResolver); | ||
| } | ||
|
Comment on lines
+730
to
+738
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: we use boxed types (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
|
||
| @Override | ||
| public SdkAsyncHttpClient buildWithDefaults(AttributeMap serviceDefaults) { | ||
| if (standardOptions.get(SdkHttpConfigurationOption.TLS_NEGOTIATION_TIMEOUT) == null) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,11 +19,13 @@ | |
| import io.netty.channel.ChannelFactory; | ||
| import io.netty.channel.EventLoopGroup; | ||
| import io.netty.channel.nio.NioEventLoopGroup; | ||
| import io.netty.channel.socket.DatagramChannel; | ||
| import io.netty.channel.socket.nio.NioDatagramChannel; | ||
| import io.netty.channel.socket.nio.NioSocketChannel; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.ThreadFactory; | ||
| import software.amazon.awssdk.annotations.SdkPublicApi; | ||
| import software.amazon.awssdk.http.nio.netty.internal.utils.SocketChannelResolver; | ||
| import software.amazon.awssdk.http.nio.netty.internal.utils.ChannelResolver; | ||
| import software.amazon.awssdk.utils.ThreadFactoryBuilder; | ||
| import software.amazon.awssdk.utils.Validate; | ||
|
|
||
|
|
@@ -39,7 +41,8 @@ | |
| * | ||
| * <li>Using {@link #create(EventLoopGroup)} to provide a custom {@link EventLoopGroup}. {@link ChannelFactory} will | ||
| * be resolved based on the type of {@link EventLoopGroup} provided via | ||
| * {@link SocketChannelResolver#resolveSocketChannelFactory(EventLoopGroup)} | ||
| * {@link ChannelResolver#resolveSocketChannelFactory(EventLoopGroup)} and | ||
| * {@link ChannelResolver#resolveDatagramChannelFactory(EventLoopGroup)} | ||
| * </li> | ||
| * | ||
| * <li>Using {@link #create(EventLoopGroup, ChannelFactory)} to provide a custom {@link EventLoopGroup} and | ||
|
|
@@ -63,20 +66,23 @@ public final class SdkEventLoopGroup { | |
|
|
||
| private final EventLoopGroup eventLoopGroup; | ||
| private final ChannelFactory<? extends Channel> channelFactory; | ||
| private final ChannelFactory<? extends DatagramChannel> datagramChannelFactory; | ||
|
|
||
| SdkEventLoopGroup(EventLoopGroup eventLoopGroup, ChannelFactory<? extends Channel> channelFactory) { | ||
| Validate.paramNotNull(eventLoopGroup, "eventLoopGroup"); | ||
| Validate.paramNotNull(channelFactory, "channelFactory"); | ||
| this.eventLoopGroup = eventLoopGroup; | ||
| this.channelFactory = channelFactory; | ||
| this.datagramChannelFactory = ChannelResolver.resolveDatagramChannelFactory(eventLoopGroup); | ||
| } | ||
|
|
||
| /** | ||
| * Create an instance of {@link SdkEventLoopGroup} from the builder | ||
| */ | ||
| private SdkEventLoopGroup(DefaultBuilder builder) { | ||
| this.eventLoopGroup = resolveEventLoopGroup(builder); | ||
| this.channelFactory = resolveChannelFactory(); | ||
| this.channelFactory = resolveSocketChannelFactory(builder); | ||
| this.datagramChannelFactory = resolveDatagramChannelFactory(builder); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -93,6 +99,13 @@ public ChannelFactory<? extends Channel> channelFactory() { | |
| return channelFactory; | ||
| } | ||
|
|
||
| /** | ||
| * @return the {@link ChannelFactory} for datagram channels to be used with Netty Http Client. | ||
| */ | ||
| public ChannelFactory<? extends DatagramChannel> datagramChannelFactory() { | ||
| return datagramChannelFactory; | ||
| } | ||
|
Comment on lines
+114
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do It would also avoid the overhead of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That wouldn't be possible, I tried changing the signature but, as an example,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, now I see what you meant. Alright, changes pushed. |
||
|
|
||
| /** | ||
| * Creates a new instance of SdkEventLoopGroup with {@link EventLoopGroup} and {@link ChannelFactory} | ||
| * to be used with {@link NettyNioAsyncHttpClient}. | ||
|
|
@@ -116,7 +129,7 @@ public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup, ChannelFac | |
| * @return a new instance of SdkEventLoopGroup | ||
| */ | ||
| public static SdkEventLoopGroup create(EventLoopGroup eventLoopGroup) { | ||
| return create(eventLoopGroup, SocketChannelResolver.resolveSocketChannelFactory(eventLoopGroup)); | ||
| return create(eventLoopGroup, ChannelResolver.resolveSocketChannelFactory(eventLoopGroup)); | ||
| } | ||
|
|
||
| public static Builder builder() { | ||
|
|
@@ -141,11 +154,22 @@ private EventLoopGroup resolveEventLoopGroup(DefaultBuilder builder) { | |
| }*/ | ||
| } | ||
|
|
||
| private ChannelFactory<? extends Channel> resolveChannelFactory() { | ||
| // Currently we only support NioEventLoopGroup | ||
| private ChannelFactory<? extends Channel> resolveSocketChannelFactory(DefaultBuilder builder) { | ||
| return builder.channelFactory; | ||
| } | ||
|
|
||
| private ChannelFactory<? extends DatagramChannel> resolveDatagramChannelFactory(DefaultBuilder builder) { | ||
| return builder.datagramChannelFactory; | ||
| } | ||
|
|
||
| private static ChannelFactory<? extends Channel> defaultSocketChannelFactory() { | ||
| return NioSocketChannel::new; | ||
| } | ||
|
|
||
| private static ChannelFactory<? extends DatagramChannel> defaultDatagramChannelFactory() { | ||
| return NioDatagramChannel::new; | ||
| } | ||
|
|
||
| /** | ||
| * A builder for {@link SdkEventLoopGroup}. | ||
| * | ||
|
|
@@ -172,13 +196,33 @@ public interface Builder { | |
| */ | ||
| Builder threadFactory(ThreadFactory threadFactory); | ||
|
|
||
| /** | ||
| * {@link ChannelFactory} to create socket channels used by the {@link EventLoopGroup}. If not set, | ||
| * NioSocketChannel is used. | ||
| * | ||
| * @param channelFactory ChannelFactory to use. | ||
| * @return This builder for method chaining. | ||
| */ | ||
| Builder channelFactory(ChannelFactory<? extends Channel> channelFactory); | ||
|
|
||
| /** | ||
| * {@link ChannelFactory} to create datagram channels used by the {@link EventLoopGroup}. If not set, | ||
| * NioDatagramChannel is used. | ||
| * | ||
| * @param datagramChannelFactory ChannelFactory to use. | ||
| * @return This builder for method chaining. | ||
| */ | ||
| Builder datagramChannelFactory(ChannelFactory<? extends DatagramChannel> datagramChannelFactory); | ||
|
|
||
| SdkEventLoopGroup build(); | ||
| } | ||
|
|
||
| private static final class DefaultBuilder implements Builder { | ||
|
|
||
| private Integer numberOfThreads; | ||
| private ThreadFactory threadFactory; | ||
| private ChannelFactory<? extends Channel> channelFactory = defaultSocketChannelFactory(); | ||
| private ChannelFactory<? extends DatagramChannel> datagramChannelFactory = defaultDatagramChannelFactory(); | ||
|
|
||
| private DefaultBuilder() { | ||
| } | ||
|
|
@@ -203,6 +247,26 @@ public void setThreadFactory(ThreadFactory threadFactory) { | |
| threadFactory(threadFactory); | ||
| } | ||
|
|
||
| @Override | ||
| public Builder channelFactory(ChannelFactory<? extends Channel> channelFactory) { | ||
| this.channelFactory = channelFactory; | ||
| return this; | ||
| } | ||
|
|
||
| public void setChannelFactory(ChannelFactory<? extends Channel> channelFactory) { | ||
| channelFactory(channelFactory); | ||
| } | ||
|
|
||
| @Override | ||
| public Builder datagramChannelFactory(ChannelFactory<? extends DatagramChannel> datagramChannelFactory) { | ||
| this.datagramChannelFactory = datagramChannelFactory; | ||
| return this; | ||
| } | ||
|
|
||
| public void setDatagramChannelFactory(ChannelFactory<? extends DatagramChannel> datagramChannelFactory) { | ||
| datagramChannelFactory(datagramChannelFactory); | ||
| } | ||
|
|
||
| @Override | ||
| public SdkEventLoopGroup build() { | ||
| return new SdkEventLoopGroup(this); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"). | ||
| * You may not use this file except in compliance with the License. | ||
| * A copy of the License is located at | ||
| * | ||
| * http://aws.amazon.com/apache2.0 | ||
| * | ||
| * or in the "license" file accompanying this file. This file 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 software.amazon.awssdk.http.nio.netty.internal; | ||
|
|
||
| import io.netty.channel.ChannelFactory; | ||
| import io.netty.channel.socket.DatagramChannel; | ||
| import io.netty.resolver.AddressResolverGroup; | ||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.lang.reflect.Method; | ||
| import java.net.InetSocketAddress; | ||
| import software.amazon.awssdk.annotations.SdkProtectedApi; | ||
| import software.amazon.awssdk.utils.ClassLoaderHelper; | ||
|
|
||
| /** | ||
| * Utility class for instantiating netty dns resolvers only if they're available on the class path. | ||
| */ | ||
| @SdkProtectedApi | ||
| public class DnsResolverLoader { | ||
|
|
||
| private DnsResolverLoader() { | ||
| } | ||
|
|
||
| public static AddressResolverGroup<InetSocketAddress> init(ChannelFactory<? extends DatagramChannel> datagramChannelFactory) { | ||
| try { | ||
| Class<?> addressResolver = ClassLoaderHelper.loadClass(getAddressResolverGroup(), false, (Class) null); | ||
| Class<?> dnsNameResolverBuilder = ClassLoaderHelper.loadClass(getDnsNameResolverBuilder(), false, (Class) null); | ||
|
|
||
| Object dnsResolverObj = dnsNameResolverBuilder.newInstance(); | ||
| Method method = dnsResolverObj.getClass().getMethod("channelFactory", ChannelFactory.class); | ||
| method.invoke(dnsResolverObj, datagramChannelFactory); | ||
|
|
||
| Object e = addressResolver.getConstructor(dnsNameResolverBuilder).newInstance(dnsResolverObj); | ||
| return (AddressResolverGroup<InetSocketAddress>) e; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't specify a DNS Caching policy, which means the default will be used. |
||
| } catch (ClassNotFoundException e) { | ||
| throw new IllegalStateException("Cannot find module io.netty.resolver.dns " | ||
| + " To use netty non blocking dns," + | ||
| " the 'netty-resolver-dns' module from io.netty must be on the class path. ", e); | ||
| } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException | InstantiationException e) { | ||
| throw new IllegalStateException("Failed to create AddressResolverGroup", e); | ||
| } | ||
| } | ||
|
|
||
| private static String getAddressResolverGroup() { | ||
| return "io.netty.resolver.dns.DnsAddressResolverGroup"; | ||
| } | ||
|
|
||
| private static String getDnsNameResolverBuilder() { | ||
| return "io.netty.resolver.dns.DnsNameResolverBuilder"; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an extra 161 KB, for a feature that's off by default.
It should probably be
optionalfor now, with updated javadoc to specify that it requires an extra dependency onnetty-resolver-dns. Can we also update the error message they get if they try to enable it without the extra dependency to something like "hey, you forgot to depend on io.netty:netty-resolver-dns"? Prior art:aws-sdk-java-v2/core/auth/src/main/java/software/amazon/awssdk/auth/signer/SignerLoader.java
Lines 56 to 58 in 7d302ee
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While implementing this, I stumped upon a decision: I want to reuse the
ClassLoaderHelper.javaclass used in SignerLoader and other classes to dynamically instantiate dependencies. The problem is that this loader resides in the sdk-core module.On the other hand, netty-nio-client does not declare this sdk module. Now I don't now if it's worth to include this dependency, if it is heavier than 161 KB then it beats the purpouse. I don't know how heavy it is, should I include it?
Other solution I thought would be just duplicate a small portion of
ClassLoaderHelper.javainside the netty-nio-client, just the necessary to load classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we certainly can't add the dependency, since it would create a cyclic relationship. Honestly, the
authmodule shouldn't even be depending onClassLoaderHelper, since it's a module-internal API.Can you create
ClassLoaderHelperin theutilsmodule and use it from there?We should go back and move all uses of
ClassLoaderHelperfrom thesdk-corecopy to theutilsmodule, and deprecate-for-removal the one insdk-core. I won't ask you to do the moving-everything-else part of it, since that seems like it should be a separate PR.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I can create create
ClassLoaderHelperwithinutilsmodule.By the way, I will be away for the rest of the week, so I expect to continue on this PR starting next week. Looking forward to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.