From 075cf8c02e005d61f424137d1b786b2ac669998e Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 15 Apr 2019 21:42:04 +0200 Subject: [PATCH] DnsNameResolver.resolve(...) should notify future as soon as one preferred record was resolved (#9050) Motivation: At the moment resolve(...) does just delegate to resolveAll(...) and so will only notify the future once all records were resolved. This is wasteful as we are only interested in the first record anyway. We should notify the promise as soon as one record that matches the preferred record type is resolved. Modifications: - Introduce DnsResolveContext.isCompleteEarly(...) to be able to detect once we should early notify the promise. - Make use of this early detecting if resolve(...) is called - Remove FutureListener which could lead to IllegalReferenceCountException due double releases - add unit test Result: Be able to notify about resolved host more quickly. --- .../dns/DnsAddressResolveContext.java | 12 ++- .../netty/resolver/dns/DnsNameResolver.java | 26 ++++--- .../resolver/dns/DnsRecordResolveContext.java | 5 ++ .../netty/resolver/dns/DnsResolveContext.java | 73 ++++++++++++------- .../resolver/dns/DnsNameResolverTest.java | 25 +++++++ 5 files changed, 99 insertions(+), 42 deletions(-) diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java index a7c780eeac..3068ac6b66 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java @@ -31,13 +31,16 @@ final class DnsAddressResolveContext extends DnsResolveContext { private final DnsCache resolveCache; private final AuthoritativeDnsServerCache authoritativeDnsServerCache; + private final boolean completeEarlyIfPossible; DnsAddressResolveContext(DnsNameResolver parent, String hostname, DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs, DnsCache resolveCache, - AuthoritativeDnsServerCache authoritativeDnsServerCache) { + AuthoritativeDnsServerCache authoritativeDnsServerCache, + boolean completeEarlyIfPossible) { super(parent, hostname, DnsRecord.CLASS_IN, parent.resolveRecordTypes(), additionals, nameServerAddrs); this.resolveCache = resolveCache; this.authoritativeDnsServerCache = authoritativeDnsServerCache; + this.completeEarlyIfPossible = completeEarlyIfPossible; } @Override @@ -46,7 +49,7 @@ final class DnsAddressResolveContext extends DnsResolveContext { DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs) { return new DnsAddressResolveContext(parent, hostname, additionals, nameServerAddrs, resolveCache, - authoritativeDnsServerCache); + authoritativeDnsServerCache, completeEarlyIfPossible); } @Override @@ -60,6 +63,11 @@ final class DnsAddressResolveContext extends DnsResolveContext { return unfiltered; } + @Override + boolean isCompleteEarly(InetAddress resolved) { + return completeEarlyIfPossible && parent.preferredAddressType().addressType() == resolved.getClass(); + } + @Override void cache(String hostname, DnsRecord[] additionals, DnsRecord result, InetAddress convertedResult) { diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java index 0d37263202..3d6e3b129e 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java @@ -851,7 +851,7 @@ public class DnsNameResolver extends InetNameResolver { } if (!doResolveCached(hostname, additionals, promise, resolveCache)) { - doResolveUncached(hostname, additionals, promise, resolveCache); + doResolveUncached(hostname, additionals, promise, resolveCache, true); } } @@ -905,9 +905,9 @@ public class DnsNameResolver extends InetNameResolver { private void doResolveUncached(String hostname, DnsRecord[] additionals, final Promise promise, - DnsCache resolveCache) { + DnsCache resolveCache, boolean completeEarlyIfPossible) { final Promise> allPromise = executor().newPromise(); - doResolveAllUncached(hostname, additionals, allPromise, resolveCache); + doResolveAllUncached(hostname, additionals, allPromise, resolveCache, true); allPromise.addListener(new FutureListener>() { @Override public void operationComplete(Future> future) { @@ -954,7 +954,7 @@ public class DnsNameResolver extends InetNameResolver { } if (!doResolveAllCached(hostname, additionals, promise, resolveCache, resolvedInternetProtocolFamilies)) { - doResolveAllUncached(hostname, additionals, promise, resolveCache); + doResolveAllUncached(hostname, additionals, promise, resolveCache, false); } } @@ -997,33 +997,35 @@ public class DnsNameResolver extends InetNameResolver { private void doResolveAllUncached(final String hostname, final DnsRecord[] additionals, final Promise> promise, - final DnsCache resolveCache) { + final DnsCache resolveCache, + final boolean completeEarlyIfPossible) { // Call doResolveUncached0(...) in the EventLoop as we may need to submit multiple queries which would need // to submit multiple Runnable at the end if we are not already on the EventLoop. EventExecutor executor = executor(); if (executor.inEventLoop()) { - doResolveAllUncached0(hostname, additionals, promise, resolveCache); + doResolveAllUncached0(hostname, additionals, promise, resolveCache, completeEarlyIfPossible); } else { executor.execute(new Runnable() { @Override public void run() { - doResolveAllUncached0(hostname, additionals, promise, resolveCache); + doResolveAllUncached0(hostname, additionals, promise, resolveCache, completeEarlyIfPossible); } }); } } private void doResolveAllUncached0(String hostname, - DnsRecord[] additionals, - Promise> promise, - DnsCache resolveCache) { + DnsRecord[] additionals, + Promise> promise, + DnsCache resolveCache, + boolean completeEarlyIfPossible) { assert executor().inEventLoop(); final DnsServerAddressStream nameServerAddrs = dnsServerAddressStreamProvider.nameServerAddressStream(hostname); - new DnsAddressResolveContext(this, hostname, additionals, nameServerAddrs, - resolveCache, authoritativeDnsServerCache).resolve(promise); + new DnsAddressResolveContext(this, hostname, additionals, nameServerAddrs, resolveCache, + authoritativeDnsServerCache, completeEarlyIfPossible).resolve(promise); } private static String hostname(String inetHost) { diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java index e633eb8820..40f213f8a1 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java @@ -58,6 +58,11 @@ final class DnsRecordResolveContext extends DnsResolveContext { return unfiltered; } + @Override + boolean isCompleteEarly(DnsRecord resolved) { + return false; + } + @Override void cache(String hostname, DnsRecord[] additionals, DnsRecord result, DnsRecord convertedResult) { // Do not cache. diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java index 5c5a6291b0..09a73d5dc4 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java @@ -62,15 +62,6 @@ import static java.lang.Math.min; abstract class DnsResolveContext { - private static final FutureListener> RELEASE_RESPONSE = - new FutureListener>() { - @Override - public void operationComplete(Future> future) { - if (future.isSuccess()) { - future.getNow().release(); - } - } - }; private static final RuntimeException NXDOMAIN_QUERY_FAILED_EXCEPTION = ThrowableUtil.unknownStackTrace( new RuntimeException("No answer found and NXDOMAIN response code returned"), DnsResolveContext.class, @@ -107,6 +98,7 @@ abstract class DnsResolveContext { private List finalResult; private int allowedQueries; private boolean triedCNAME; + private boolean completeEarly; DnsResolveContext(DnsNameResolver parent, String hostname, int dnsClass, DnsRecordType[] expectedTypes, @@ -165,6 +157,8 @@ abstract class DnsResolveContext { */ abstract List filterResults(List unfiltered); + abstract boolean isCompleteEarly(T resolved); + /** * Caches a successful resolution. */ @@ -329,7 +323,8 @@ abstract class DnsResolveContext { final boolean flush, final Promise> promise, final Throwable cause) { - if (nameServerAddrStreamIndex >= nameServerAddrStream.size() || allowedQueries == 0 || promise.isCancelled()) { + if (completeEarly || nameServerAddrStreamIndex >= nameServerAddrStream.size() || + allowedQueries == 0 || promise.isCancelled()) { tryToFinishResolve(nameServerAddrStream, nameServerAddrStreamIndex, question, queryLifecycleObserver, promise, cause); return; @@ -456,7 +451,7 @@ abstract class DnsResolveContext { public boolean clear(String hostname) { return authoritativeDnsServerCache.clear(hostname); } - }).resolve(resolverPromise); + }, false).resolve(resolverPromise); } } @@ -695,21 +690,41 @@ abstract class DnsResolveContext { continue; } - // We want to ensure we do not have duplicates in finalResult as this may be unexpected. - // - // While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an - // ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay - // for the extra memory copy and allocations in this cases later on. - if (finalResult == null) { - finalResult = new ArrayList(8); - finalResult.add(converted); - } else if (!finalResult.contains(converted)) { - finalResult.add(converted); + boolean shouldRelease = false; + // Check if we did determine we wanted to complete early before. If this is the case we want to not + // include the result + if (!completeEarly) { + boolean completeEarly = isCompleteEarly(converted); + if (completeEarly) { + this.completeEarly = true; + } + // We want to ensure we do not have duplicates in finalResult as this may be unexpected. + // + // While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an + // ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay + // for the extra memory copy and allocations in this cases later on. + if (finalResult == null) { + if (completeEarly) { + finalResult = Collections.singletonList(converted); + } else { + finalResult = new ArrayList(8); + finalResult.add(converted); + } + } else if (!finalResult.contains(converted)) { + finalResult.add(converted); + } else { + shouldRelease = true; + } + } else { + shouldRelease = true; } cache(hostname, additionals, r, converted); found = true; + if (shouldRelease) { + ReferenceCountUtil.release(converted); + } // Note that we do not break from the loop here, so we decode/cache all A/AAAA records. } @@ -800,7 +815,7 @@ abstract class DnsResolveContext { final Throwable cause) { // There are no queries left to try. - if (!queriesInProgress.isEmpty()) { + if (!completeEarly && !queriesInProgress.isEmpty()) { queryLifecycleObserver.queryCancelled(allowedQueries); // There are still some queries in process, we will try to notify once the next one finishes until @@ -846,22 +861,24 @@ abstract class DnsResolveContext { } private void finishResolve(Promise> promise, Throwable cause) { - if (!queriesInProgress.isEmpty()) { + // If completeEarly was true we still want to continue processing the queries to ensure we still put everything + // in the cache eventually. + if (!completeEarly && !queriesInProgress.isEmpty()) { // If there are queries in progress, we should cancel it because we already finished the resolution. for (Iterator>> i = queriesInProgress.iterator(); i.hasNext();) { Future> f = i.next(); i.remove(); - if (!f.cancel(false)) { - f.addListener(RELEASE_RESPONSE); - } + f.cancel(false); } } if (finalResult != null) { - // Found at least one resolved record. - DnsNameResolver.trySuccess(promise, filterResults(finalResult)); + if (!promise.isDone()) { + // Found at least one resolved record. + DnsNameResolver.trySuccess(promise, filterResults(finalResult)); + } return; } diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java index 87458afe4e..605f24adac 100644 --- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java +++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java @@ -2620,4 +2620,29 @@ public class DnsNameResolverTest { } } } + + @Test(timeout = 2000) + public void testDropAAAAResolveFastFast() throws IOException { + String host = "somehost.netty.io"; + TestDnsServer dnsServer2 = new TestDnsServer(Collections.singleton(host)); + dnsServer2.start(true); + DnsNameResolver resolver = null; + try { + DnsNameResolverBuilder builder = newResolver() + .recursionDesired(false) + .queryTimeoutMillis(10000) + .resolvedAddressTypes(ResolvedAddressTypes.IPV4_PREFERRED) + .maxQueriesPerResolve(16) + .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer2.localAddress())); + + resolver = builder.build(); + InetAddress address = resolver.resolve(host).syncUninterruptibly().getNow(); + assertEquals(host, address.getHostName()); + } finally { + dnsServer2.stop(); + if (resolver != null) { + resolver.close(); + } + } + } }