From 12a413bf024e64946b28ed9aa3065a31b9420ad2 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 15 Nov 2017 18:49:47 +0100 Subject: [PATCH] Allow to detect failed query caused by an Timeout / IO error and also not cache these. Motivation: At the moment there is not way for the user to know if resolving a domain was failed because the domain was unkown or because of an IO error / timeout. If it was caused by an timeout / IO error the user may want to retry the query. Also if the query was failed because of an IO error / timeout we should not cache it. Modifications: - Add DnsNameResolverTimeoutException and include it in the UnkownHostException if the domain could not be resolved because of an timeout. This will allow the user to retry the query when inspecting the cause. - Do not cache IO errors / timeouts - Add unit test Result: Easier for users to implement retries for DNS querys and not cache IO errors / timeouts. --- .../java/io/netty/resolver/dns/DnsCache.java | 2 + .../netty/resolver/dns/DnsNameResolver.java | 18 +++++ .../resolver/dns/DnsNameResolverContext.java | 68 +++++++++++-------- .../dns/DnsNameResolverException.java | 2 +- .../dns/DnsNameResolverTimeoutException.java | 35 ++++++++++ .../netty/resolver/dns/DnsQueryContext.java | 9 +-- .../resolver/dns/DnsNameResolverTest.java | 47 +++++++++++++ 7 files changed, 148 insertions(+), 33 deletions(-) create mode 100644 resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverTimeoutException.java diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsCache.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsCache.java index 8564f525ed..28e8989e6e 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsCache.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsCache.java @@ -65,6 +65,8 @@ public interface DnsCache { /** * Cache the resolution failure for a given hostname. + * Be aware this won't be called with timeout / cancel / transport exceptions. + * * @param hostname the hostname * @param additionals the additional records * @param cause the resolution failure 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 da76a8897a..8510488c68 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 @@ -891,6 +891,24 @@ public class DnsNameResolver extends InetNameResolver { return query0(nameServerAddr, question, toArray(additionals, false), promise); } + /** + * Returns {@code true} if the {@link Throwable} was caused by an timeout or transport error. + * These methods can be used on the {@link Future#cause()} that is returned by the various methods exposed by this + * {@link DnsNameResolver}. + */ + public static boolean isTransportOrTimeoutError(Throwable cause) { + return cause != null && cause.getCause() instanceof DnsNameResolverException; + } + + /** + * Returns {@code true} if the {@link Throwable} was caused by an timeout. + * These methods can be used on the {@link Future#cause()} that is returned by the various methods exposed by this + * {@link DnsNameResolver}. + */ + public static boolean isTimeoutError(Throwable cause) { + return cause != null && cause.getCause() instanceof DnsNameResolverTimeoutException; + } + final Future> query0( InetSocketAddress nameServerAddr, DnsQuestion question, DnsRecord[] additionals, diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java index 389693bc40..5d9f71b2c7 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java @@ -189,11 +189,11 @@ abstract class DnsNameResolverContext { assert recordTypes.length > 0; final int end = recordTypes.length - 1; for (int i = 0; i < end; ++i) { - if (!query(hostname, recordTypes[i], nameServerAddressStream.duplicate(), promise)) { + if (!query(hostname, recordTypes[i], nameServerAddressStream.duplicate(), promise, null)) { return; } } - query(hostname, recordTypes[end], nameServerAddressStream, promise); + query(hostname, recordTypes[end], nameServerAddressStream, promise, null); } /** @@ -283,19 +283,20 @@ abstract class DnsNameResolverContext { private void query(final DnsServerAddressStream nameServerAddrStream, final int nameServerAddrStreamIndex, final DnsQuestion question, - final Promise promise) { + final Promise promise, Throwable cause) { query(nameServerAddrStream, nameServerAddrStreamIndex, question, - parent.dnsQueryLifecycleObserverFactory().newDnsQueryLifecycleObserver(question), promise); + parent.dnsQueryLifecycleObserverFactory().newDnsQueryLifecycleObserver(question), promise, cause); } private void query(final DnsServerAddressStream nameServerAddrStream, final int nameServerAddrStreamIndex, final DnsQuestion question, final DnsQueryLifecycleObserver queryLifecycleObserver, - final Promise promise) { + final Promise promise, + final Throwable cause) { if (nameServerAddrStreamIndex >= nameServerAddrStream.size() || allowedQueries == 0 || promise.isCancelled()) { tryToFinishResolve(nameServerAddrStream, nameServerAddrStreamIndex, question, queryLifecycleObserver, - promise); + promise, cause); return; } @@ -319,21 +320,22 @@ abstract class DnsNameResolverContext { return; } + final Throwable queryCause = future.cause(); try { - if (future.isSuccess()) { + if (queryCause == null) { onResponse(nameServerAddrStream, nameServerAddrStreamIndex, question, future.getNow(), queryLifecycleObserver, promise); } else { // Server did not respond or I/O error occurred; try again. - queryLifecycleObserver.queryFailed(future.cause()); - query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, promise); + queryLifecycleObserver.queryFailed(queryCause); + query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, promise, queryCause); } } finally { tryToFinishResolve(nameServerAddrStream, nameServerAddrStreamIndex, question, // queryLifecycleObserver has already been terminated at this point so we must // not allow it to be terminated again by tryToFinishResolve. NoopDnsQueryLifecycleObserver.INSTANCE, - promise); + promise, queryCause); } } }); @@ -366,7 +368,7 @@ abstract class DnsNameResolverContext { // Retry with the next server if the server did not tell us that the domain does not exist. if (code != DnsResponseCode.NXDOMAIN) { query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, - queryLifecycleObserver.queryNoAnswer(code), promise); + queryLifecycleObserver.queryNoAnswer(code), promise, null); } else { queryLifecycleObserver.queryFailed(NXDOMAIN_QUERY_FAILED_EXCEPTION); } @@ -420,7 +422,7 @@ abstract class DnsNameResolverContext { if (!nameServers.isEmpty()) { query(parent.uncachedRedirectDnsServerStream(nameServers), 0, question, - queryLifecycleObserver.queryRedirected(unmodifiableList(nameServers)), promise); + queryLifecycleObserver.queryRedirected(unmodifiableList(nameServers)), promise, null); return true; } } @@ -601,7 +603,8 @@ abstract class DnsNameResolverContext { final int nameServerAddrStreamIndex, final DnsQuestion question, final DnsQueryLifecycleObserver queryLifecycleObserver, - final Promise promise) { + final Promise promise, + final Throwable cause) { // There are no queries left to try. if (!queriesInProgress.isEmpty()) { queryLifecycleObserver.queryCancelled(allowedQueries); @@ -609,7 +612,7 @@ abstract class DnsNameResolverContext { // There are still some queries we did not receive responses for. if (gotPreferredAddress()) { // But it's OK to finish the resolution process if we got a resolved address of the preferred type. - finishResolve(promise); + finishResolve(promise, cause); } // We did not get any resolved address of the preferred type, so we can't finish the resolution process. @@ -622,10 +625,10 @@ abstract class DnsNameResolverContext { if (queryLifecycleObserver == NoopDnsQueryLifecycleObserver.INSTANCE) { // If the queryLifecycleObserver has already been terminated we should create a new one for this // fresh query. - query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, promise); + query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, promise, cause); } else { query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, queryLifecycleObserver, - promise); + promise, cause); } return; } @@ -633,11 +636,15 @@ abstract class DnsNameResolverContext { queryLifecycleObserver.queryFailed(NAME_SERVERS_EXHAUSTED_EXCEPTION); // .. and we could not find any A/AAAA records. - if (!triedCNAME) { + + // If cause != null we know this was caused by a timeout / cancel / transport exception. In this case we + // won't try to resolve the CNAME as we only should do this if we could not get the A/AAAA records because + // these not exists and the DNS server did probably signal it. + if (cause == null && !triedCNAME) { // As the last resort, try to query CNAME, just in case the name server has it. triedCNAME = true; - query(hostname, DnsRecordType.CNAME, getNameServers(hostname), promise); + query(hostname, DnsRecordType.CNAME, getNameServers(hostname), promise, null); return; } } else { @@ -645,7 +652,7 @@ abstract class DnsNameResolverContext { } // We have at least one resolved address or tried CNAME as the last resort.. - finishResolve(promise); + finishResolve(promise, cause); } private boolean gotPreferredAddress() { @@ -664,7 +671,7 @@ abstract class DnsNameResolverContext { return false; } - private void finishResolve(Promise promise) { + private void finishResolve(Promise promise, Throwable cause) { if (!queriesInProgress.isEmpty()) { // If there are queries in progress, we should cancel it because we already finished the resolution. for (Iterator>> i = queriesInProgress.iterator(); @@ -703,10 +710,15 @@ abstract class DnsNameResolverContext { .append(' '); } } - final UnknownHostException cause = new UnknownHostException(buf.toString()); - - resolveCache.cache(hostname, additionals, cause, parent.ch.eventLoop()); - promise.tryFailure(cause); + final UnknownHostException unknownHostException = new UnknownHostException(buf.toString()); + if (cause == null) { + // Only cache if the failure was not because of an IO error / timeout that was caused by the query + // itself. + resolveCache.cache(hostname, additionals, unknownHostException, parent.ch.eventLoop()); + } else { + unknownHostException.initCause(cause); + } + promise.tryFailure(unknownHostException); } abstract boolean finishResolve(Class addressType, List resolvedEntries, @@ -747,7 +759,7 @@ abstract class DnsNameResolverContext { queryLifecycleObserver.queryFailed(cause); PlatformDependent.throwException(cause); } - query(stream, 0, cnameQuestion, queryLifecycleObserver.queryCNAMEd(cnameQuestion), promise); + query(stream, 0, cnameQuestion, queryLifecycleObserver.queryCNAMEd(cnameQuestion), promise, null); } if (parent.supportsAAAARecords()) { try { @@ -758,17 +770,17 @@ abstract class DnsNameResolverContext { queryLifecycleObserver.queryFailed(cause); PlatformDependent.throwException(cause); } - query(stream, 0, cnameQuestion, queryLifecycleObserver.queryCNAMEd(cnameQuestion), promise); + query(stream, 0, cnameQuestion, queryLifecycleObserver.queryCNAMEd(cnameQuestion), promise, null); } } private boolean query(String hostname, DnsRecordType type, DnsServerAddressStream dnsServerAddressStream, - Promise promise) { + Promise promise, Throwable cause) { final DnsQuestion question = newQuestion(hostname, type); if (question == null) { return false; } - query(dnsServerAddressStream, 0, question, promise); + query(dnsServerAddressStream, 0, question, promise, cause); return true; } diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverException.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverException.java index 07eb32e63c..77e347449b 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverException.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverException.java @@ -26,7 +26,7 @@ import java.net.InetSocketAddress; * A {@link RuntimeException} raised when {@link DnsNameResolver} failed to perform a successful query. */ @UnstableApi -public final class DnsNameResolverException extends RuntimeException { +public class DnsNameResolverException extends RuntimeException { private static final long serialVersionUID = -8826717909627131850L; diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverTimeoutException.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverTimeoutException.java new file mode 100644 index 0000000000..e1ad13de4d --- /dev/null +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverTimeoutException.java @@ -0,0 +1,35 @@ +/* + * Copyright 2017 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License 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 io.netty.resolver.dns; + +import io.netty.handler.codec.dns.DnsQuestion; +import io.netty.util.internal.UnstableApi; + +import java.net.InetSocketAddress; + +/** + * A {@link DnsNameResolverException} raised when {@link DnsNameResolver} failed to perform a successful query because + * of an timeout. In this case you may want to retry the operation. + */ +@UnstableApi +public final class DnsNameResolverTimeoutException extends DnsNameResolverException { + private static final long serialVersionUID = -8826717969627131854L; + + public DnsNameResolverTimeoutException( + InetSocketAddress remoteAddress, DnsQuestion question, String message) { + super(remoteAddress, question, message); + } +} diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java index 4f8707701f..12196f184c 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java @@ -213,12 +213,13 @@ final class DnsQueryContext { .append(" (no stack trace available)"); final DnsNameResolverException e; - if (cause != null) { - e = new DnsNameResolverException(nameServerAddr, question(), buf.toString(), cause); + if (cause == null) { + // This was caused by an timeout so use DnsNameResolverTimeoutException to allow the user to + // handle it special (like retry the query). + e = new DnsNameResolverTimeoutException(nameServerAddr, question(), buf.toString()); } else { - e = new DnsNameResolverException(nameServerAddr, question(), buf.toString()); + e = new DnsNameResolverException(nameServerAddr, question(), buf.toString(), cause); } - promise.tryFailure(e); } } 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 2e99308970..707a061d56 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 @@ -1322,4 +1322,51 @@ public class DnsNameResolverTest { return rm.getEntry(); } } + + @Test(timeout = 3000) + public void testTimeoutNotCached() { + DnsCache cache = new DnsCache() { + @Override + public void clear() { + // NOOP + } + + @Override + public boolean clear(String hostname) { + return false; + } + + @Override + public List get(String hostname, DnsRecord[] additionals) { + return Collections.emptyList(); + } + + @Override + public DnsCacheEntry cache(String hostname, DnsRecord[] additionals, InetAddress address, + long originalTtl, EventLoop loop) { + fail("Should not be cached"); + return null; + } + + @Override + public DnsCacheEntry cache(String hostname, DnsRecord[] additionals, Throwable cause, EventLoop loop) { + fail("Should not be cached"); + return null; + } + }; + DnsNameResolverBuilder builder = newResolver(); + builder.queryTimeoutMillis(100) + .authoritativeDnsServerCache(cache) + .resolveCache(cache) + .nameServerProvider(new SingletonDnsServerAddressStreamProvider( + new InetSocketAddress(NetUtil.LOCALHOST, 12345))); + DnsNameResolver resolver = builder.build(); + Future result = resolver.resolve("doesnotexist.netty.io").awaitUninterruptibly(); + Throwable cause = result.cause(); + assertTrue(cause instanceof UnknownHostException); + assertTrue(cause.getCause() instanceof DnsNameResolverTimeoutException); + assertTrue(DnsNameResolver.isTimeoutError(cause)); + assertTrue(DnsNameResolver.isTransportOrTimeoutError(cause)); + resolver.close(); + } }