Use netty nio DNS resolver by default in HttpClient #1581

johnrengelman · 2021-02-10T00:37:26Z

Allows for overriding the implementation by adding AddressResolverGroup
to the Registry.

This change is

johnrengelman · 2021-02-10T00:40:52Z

ratpack-core/src/main/java/ratpack/http/client/internal/DefaultHttpClient.java

      Bootstrap bootstrap = new Bootstrap()
        .remoteAddress(key.host, key.port)
        .group(key.execution.getEventLoop())
+        .resolver(key.execution.maybeGet(AddressResolverGroup.class)


This is the same pattern we do over in raptack-retrofit2, but it's ugly and still requires that users explicitly bind or provide an ExecInitializer that registers AddressResolverGroup to forked executions.

I changed this pattern, so that we looking in the execution and if nothing is there, we get the default netty backed resolver (which defaults its configuration to the system properties in /etc/resolv.conf). This means a user will still need to explicitly ensure that AddressResolverGroup is registered in any forked executions (including ParallelBatch) if they do any customizations.

Also, not sure what the impact of creating/gc'ing those would be....it doesn't look like there's any state really.

One oddity here, is that we are finding the AddressResolverGroup from the Execution, if you are using Guice and injecting HttpClient, the AddressResolverGroup doesn't come along with it. It's almost as though HttpClient should also inject AddressResolverGroup, but the Ratpack Registry doesn't do dependency binding.

johnrengelman · 2021-02-10T00:41:49Z

ratpack-core/src/main/java/ratpack/server/internal/ServerRegistry.java

+      DnsNameResolverBuilder resolverBuilder = new DnsNameResolverBuilder()
+        .eventLoop(execController.getEventLoopGroup().next())
+        .channelType(TransportDetector.getDatagramChannelImpl())
+        .socketChannelType(TransportDetector.getSocketChannelImpl());


This is essentially the bare minimum for the resolver.

Should we provide some type of configuration via ServerConfig to provide tunings here or just ask the user to provide their own implementation of AddressResolverGroup on the Registry?

It will be very beneficial if we can configure the dns query timeout at the least as default.
But, there is already a way to provide few configurations with this change: netty/netty@4f72cdf#diff-e7ae8540514a0e3caa415abc27aaee9fe93b7732d318d05a9534686447a162b9L146 .
Other tuning configs would become tricky to expose here just because they are internal to netty and are in plenty.

I didn't see that previously @amityatagiri, good find.
It does look like the query time is configured based on the the timeout: option in /etc/resolv.conf (https://github.com/netty/netty/blob/4.1/resolver-dns/src/main/java/io/netty/resolver/dns/UnixResolverDnsServerAddressStreamProvider.java#L334).
So if this is configurable here, what about the benefit of providing a config option inside of Ratpack?
I'd probably lean toward not providing any options here since they are configurable and obtained from the OS by default and ask the user to supply a fully configured AddressResolverGroup if they want to tweak things.

1.9.x branch is still pointing to netty 4.1.48.Final. We may want to match it with the master branch or to the latest netty.

Yes,
master is the 2.x branch right now, that's why it's updated. But we can do that updated in 1.9.x as well. @amityatagiri can you open another PR that is targeted to 1.9.x with that dependency change?

johnrengelman · 2021-02-10T00:42:55Z

ratpack-test/src/main/java/ratpack/test/internal/BlockingHttpClient.java

@@ -46,6 +48,7 @@ public ReceivedResponse request(HttpClient httpClient, URI uri, ExecController e
    AtomicReference<ExecResult<ReceivedResponse>> result = new AtomicReference<>();

    execController.fork()
+      .register(r -> r.add(AddressResolverGroup.class, DefaultAddressResolverGroup.INSTANCE))


This does the necessary registry add so that the HttpClient works, but could be removed if we a more holistic approach to this.

johnrengelman · 2021-02-10T00:43:47Z

ratpack-core/src/main/java/ratpack/server/internal/ServerRegistry.java

@@ -116,6 +125,7 @@ public static Registry buildBaseRegistry(RatpackServer ratpackServer, Imposition
        .add(Clock.class, Clock.systemDefaultZone())
        .add(RatpackServer.class, ratpackServer)
        .add(ObjectMapper.class, new ObjectMapper())
+        .add(AddressResolverGroup.class, new DnsAddressResolverGroup(resolverBuilder))


Users can switch back to the default JVM implementation by doing:

.add(AddressResolverGroup.class, DefaultAddressResolverGroup.INSTANCE)

in the user registry.

johnrengelman · 2021-02-11T15:05:11Z

ratpack-core/src/main/java/ratpack/server/internal/ServerRegistry.java

+
+  public static AddressResolverGroup<?> defaultAddressResolver(ExecController controller) {
+    DnsNameResolverBuilder resolverBuilder = new DnsNameResolverBuilder()
+      .eventLoop(controller.getEventLoopGroup().next())


This does mean that all DNS resolves are bound to 1 event loop...which, since it's NIO shouldn't be a problem (and it is how reactor-netty does it too).

Allows for overriding the implementation by adding AddressResolverGroup to the Registry.

johnrengelman · 2021-02-15T01:03:27Z

ratpack-core/src/main/java/ratpack/http/client/internal/DefaultHttpClient.java

@@ -88,6 +95,10 @@ private DefaultHttpClient(DefaultHttpClient.Spec spec) {
    } else {
      proxy = null;
    }
+    if (spec.resolver == null) {
+      LOGGER.warn("DNS resolver not specified for HttpClientSpec, defaulting to JVM blocking implementation");
+      spec.resolver = HttpClient.blockingResolver();


Do the only sane thing we can if someone is creating a new HttpClient and doesn't provide a resolver, which is use the blocking implementation but WARN about it.

We can't default to a non-blocking implementation because we don't have a handle to an EventLoop which is needed. We could defer to to the point that we make the request but that would create high GC churn and possible performance impacts as each request would create its own resolver.

johnrengelman · 2021-02-15T01:04:12Z

ratpack-core/src/main/java/ratpack/http/client/HttpClient.java

+   * @since 1.9.0
+   */
+  static AddressResolverGroup<?> addressResolver(ExecController controller, Action<? super DnsNameResolverBuilder> spec) throws Exception {
+    DnsNameResolverBuilder resolverBuilder = new DnsNameResolverBuilder()


The builder automatically loads defaults from /etc/resolv.conf.

johnrengelman · 2021-02-15T01:04:48Z

ratpack-test/src/main/java/ratpack/test/http/internal/DefaultTestHttpClient.java

@@ -256,6 +256,7 @@ private HttpClient httpClient() {
      .byteBufAllocator(TestByteBufAllocators.LEAKING_UNPOOLED_HEAP)
      .maxContentLength(Integer.MAX_VALUE)
      .poolSize(8)
+      .addressResolver(HttpClient.blockingResolver())


Since this is the blocking client for tests, use the blocking resolver and avoid the warning.

Provide necessary defaults and helper methods for configuring.

amityatagiri

LGTM

…olver

ldaley · 2021-02-20T11:55:31Z

@johnrengelman pls check my changes. I haven't observed the full CI response yet, but will do so in the next few days.

johnrengelman · 2021-02-20T20:38:52Z

@ldaley Looks good. I pushed fixes for the broken tests. Biggest piece of that was some HttpClient metrics classes that were not elevated to the public API so we were casting to DefaultHttpClient to get them. I've moved those to the public API which requires any HttpClient to now implement them.

This reverts commit 33376f7.

ldaley · 2021-02-21T23:33:54Z

@johnrengelman I reverted the move to make the stats public. It wasn't necessary, and should be done as its own change if that's what we want to do.

On that, I'm not sure it is what we want to do. As is stands, we are putting the core client on the hook for maintaining too much information by having it be responsible for providing “stats”. What we should do here instead is allow “event listeners” to be attached that can be notified of various things happening, and they can interpret that into whatever kind of stats they want and with what retention they want.

johnrengelman · 2021-02-21T23:36:51Z

Yeah. That’s a better plan. Something we should address in trunk.

This looks good to me.

johnrengelman commented Feb 10, 2021

View reviewed changes

johnrengelman added this to the release-1.9.0 milestone Feb 11, 2021

johnrengelman commented Feb 11, 2021

View reviewed changes

johnrengelman force-pushed the feature/configurable-dns-resolver branch from 9a68f92 to f237f69 Compare February 11, 2021 15:14

John Engelman added 2 commits February 14, 2021 18:14

Use netty nio DNS resolver by default in HttpClient.

28ddfbf

Allows for overriding the implementation by adding AddressResolverGroup to the Registry.

Only look in execution and create a default resolver if not found

37ac23f

johnrengelman force-pushed the feature/configurable-dns-resolver branch from f237f69 to 249b9ea Compare February 15, 2021 01:01

johnrengelman commented Feb 15, 2021

View reviewed changes

johnrengelman force-pushed the feature/configurable-dns-resolver branch 2 times, most recently from e2d0032 to 9cde3bf Compare February 15, 2021 01:41

Move AddressResolver into HttpClientSpec.

28a1ab6

Provide necessary defaults and helper methods for configuring.

johnrengelman force-pushed the feature/configurable-dns-resolver branch from 9cde3bf to 28a1ab6 Compare February 15, 2021 04:17

amityatagiri approved these changes Feb 16, 2021

View reviewed changes

Simplify API and more strongly encourage use of non blocking name res…

5f83bc7

…olver

move HttpClient metrics to public API

33376f7

ldaley added 2 commits February 22, 2021 09:22

Revert "move HttpClient metrics to public API"

11ea4ef

This reverts commit 33376f7.

Fix tests

2faff7c

runningcode and others added 3 commits February 22, 2021 09:55

Enable Gradle build cache

55c3b84

Remove lazybones integration

bf1f1b8

Add release note content.

0e01657

ldaley changed the title ~~Use netty nio DNS resolver by default in HttpClient.~~ Use netty nio DNS resolver by default in HttpClient Feb 22, 2021

ldaley merged commit e104606 into 1.9.x Feb 22, 2021

ldaley deleted the feature/configurable-dns-resolver branch February 22, 2021 00:03

johnrengelman mentioned this pull request Oct 11, 2021

Added cleaner method for creating HttpClient in test #1615

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use netty nio DNS resolver by default in HttpClient #1581

Use netty nio DNS resolver by default in HttpClient #1581

johnrengelman commented Feb 10, 2021 •

edited by ldaley

johnrengelman Feb 10, 2021 •

edited

johnrengelman Feb 11, 2021

johnrengelman Feb 10, 2021

amityatagiri Feb 10, 2021

johnrengelman Feb 10, 2021

amityatagiri Feb 11, 2021

johnrengelman Feb 11, 2021 •

edited

johnrengelman Feb 10, 2021

johnrengelman Feb 10, 2021

johnrengelman Feb 11, 2021

johnrengelman Feb 15, 2021

johnrengelman Feb 15, 2021

johnrengelman Feb 15, 2021

amityatagiri left a comment

ldaley commented Feb 20, 2021

johnrengelman commented Feb 20, 2021

ldaley commented Feb 21, 2021

johnrengelman commented Feb 21, 2021

Use netty nio DNS resolver by default in HttpClient #1581

Use netty nio DNS resolver by default in HttpClient #1581

Conversation

johnrengelman commented Feb 10, 2021 • edited by ldaley

johnrengelman Feb 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnrengelman Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amityatagiri left a comment

Choose a reason for hiding this comment

ldaley commented Feb 20, 2021

johnrengelman commented Feb 20, 2021

ldaley commented Feb 21, 2021

johnrengelman commented Feb 21, 2021

johnrengelman commented Feb 10, 2021 •

edited by ldaley

johnrengelman Feb 10, 2021 •

edited

johnrengelman Feb 11, 2021 •

edited