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.

resolver-dns/src/main/java/io/netty/resolver/dns/DnsCache.java

@@ -65,6 +65,8 @@ DnsCacheEntry cache(String hostname, DnsRecord[] additionals, InetAddress addres
 
     /**
      * Cache the resolution failure for a given hostname.
+     * Be aware this <strong>won't</strong> be called with timeout / cancel / transport exceptions.
+      *
      * @param hostname the hostname
      * @param additionals the additional records
      * @param cause the resolution failure

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java

@@ -891,6 +891,24 @@ public Future<AddressedEnvelope<DnsResponse, InetSocketAddress>> query(
         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<AddressedEnvelope<DnsResponse, InetSocketAddress>> query0(
             InetSocketAddress nameServerAddr, DnsQuestion question,
             DnsRecord[] additionals,

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java

@@ -137,14 +137,19 @@ void resolve(final Promise<T> promise) {
                 private int searchDomainIdx = initialSearchDomainIdx;
                 @Override
                 public void operationComplete(Future<T> future) throws Exception {
-                    if (future.isSuccess()) {
+                    Throwable cause = future.cause();
+                    if (cause == null) {
                         promise.trySuccess(future.getNow());
-                    } else if (searchDomainIdx < searchDomains.length) {
-                        doSearchDomainQuery(hostname + '.' + searchDomains[searchDomainIdx++], this);
-                    } else if (!startWithoutSearchDomain) {
-                        internalResolve(promise);
                     } else {
-                        promise.tryFailure(new SearchDomainUnknownHostException(future.cause(), hostname));
+                        if (DnsNameResolver.isTransportOrTimeoutError(cause)) {
+                            promise.tryFailure(new SearchDomainUnknownHostException(cause, hostname));
+                        } else if (searchDomainIdx < searchDomains.length) {
+                            doSearchDomainQuery(hostname + '.' + searchDomains[searchDomainIdx++], this);
+                        } else if (!startWithoutSearchDomain) {
+                            internalResolve(promise);
+                        } else {
+                            promise.tryFailure(new SearchDomainUnknownHostException(cause, hostname));
+                        }
                     }
                 }
             });
@@ -166,6 +171,9 @@ private static final class SearchDomainUnknownHostException extends UnknownHostE
         SearchDomainUnknownHostException(Throwable cause, String originalHostname) {
             super("Search domain query failed. Original hostname: '" + originalHostname + "' " + cause.getMessage());
             setStackTrace(cause.getStackTrace());
+
+            // Preserve the cause
+            initCause(cause.getCause());
         }
 
         @Override
@@ -189,11 +197,11 @@ private void internalResolve(Promise<T> promise) {
         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 +291,20 @@ public void remove() {
 
     private void query(final DnsServerAddressStream nameServerAddrStream, final int nameServerAddrStreamIndex,
                        final DnsQuestion question,
-                       final Promise<T> promise) {
+                       final Promise<T> 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<T> promise) {
+                       final Promise<T> promise,
+                       final Throwable cause) {
         if (nameServerAddrStreamIndex >= nameServerAddrStream.size() || allowedQueries == 0 || promise.isCancelled()) {
             tryToFinishResolve(nameServerAddrStream, nameServerAddrStreamIndex, question, queryLifecycleObserver,
-                               promise);
+                               promise, cause);
             return;
         }
 
@@ -319,21 +328,22 @@ public void operationComplete(Future<AddressedEnvelope<DnsResponse, InetSocketAd
                     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 +376,7 @@ void onResponse(final DnsServerAddressStream nameServerAddrStream, final int nam
             // 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 +430,7 @@ private boolean handleRedirect(
 
                 if (!nameServers.isEmpty()) {
                     query(parent.uncachedRedirectDnsServerStream(nameServers), 0, question,
-                          queryLifecycleObserver.queryRedirected(unmodifiableList(nameServers)), promise);
+                          queryLifecycleObserver.queryRedirected(unmodifiableList(nameServers)), promise, null);
                     return true;
                 }
             }
@@ -601,15 +611,16 @@ void tryToFinishResolve(final DnsServerAddressStream nameServerAddrStream,
                             final int nameServerAddrStreamIndex,
                             final DnsQuestion question,
                             final DnsQueryLifecycleObserver queryLifecycleObserver,
-                            final Promise<T> promise) {
+                            final Promise<T> promise,
+                            final Throwable cause) {
         // There are no queries left to try.
         if (!queriesInProgress.isEmpty()) {
             queryLifecycleObserver.queryCancelled(allowedQueries);
 
             // 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,30 +633,34 @@ void tryToFinishResolve(final DnsServerAddressStream nameServerAddrStream,
                 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;
             }
 
             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 {
             queryLifecycleObserver.queryCancelled(allowedQueries);
         }
 
         // We have at least one resolved address or tried CNAME as the last resort..
-        finishResolve(promise);
+        finishResolve(promise, cause);
     }
 
     private boolean gotPreferredAddress() {
@@ -664,7 +679,7 @@ private boolean gotPreferredAddress() {
         return false;
     }
 
-    private void finishResolve(Promise<T> promise) {
+    private void finishResolve(Promise<T> promise, Throwable cause) {
         if (!queriesInProgress.isEmpty()) {
             // If there are queries in progress, we should cancel it because we already finished the resolution.
             for (Iterator<Future<AddressedEnvelope<DnsResponse, InetSocketAddress>>> i = queriesInProgress.iterator();
@@ -703,10 +718,15 @@ private void finishResolve(Promise<T> promise) {
                 .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<? extends InetAddress> addressType, List<DnsCacheEntry> resolvedEntries,
@@ -747,7 +767,7 @@ private void followCname(String cname, final DnsQueryLifecycleObserver queryLife
                 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 +778,17 @@ private void followCname(String cname, final DnsQueryLifecycleObserver queryLife
                 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<T> promise) {
+                          Promise<T> 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;
     }
 

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverException.java

@@ -26,7 +26,7 @@
  * 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;
 

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);
+    }
+}

resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java

@@ -213,12 +213,13 @@ private void setFailure(String message, Throwable cause) {
            .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);
     }
 }

resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java

@@ -1322,4 +1322,51 @@ private static ResourceRecord newNsRecord(String dnsname, String domainName) {
             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<? extends DnsCacheEntry> 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<InetAddress> 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();
+    }
 }