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 @@ public interface DnsCache {
 
     /**
      * 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 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<AddressedEnvelope<DnsResponse, InetSocketAddress>> query0(
             InetSocketAddress nameServerAddr, DnsQuestion question,
             DnsRecord[] additionals,

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

@@ -189,11 +189,11 @@ abstract class DnsNameResolverContext<T> {
         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<T> {
 
     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 +320,22 @@ abstract class DnsNameResolverContext<T> {
                     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());
+                        queryLifecycleObserver.queryFailed(queryCause);
-                        query(nameServerAddrStream, nameServerAddrStreamIndex + 1, question, promise);
+                        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<T> {
             // 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<T> {
 
                 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<T> {
                             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);
@@ -609,7 +612,7 @@ abstract class DnsNameResolverContext<T> {
             // 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<T> {
                 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<T> {
             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<T> {
         }
 
         // 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<T> {
         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 +710,15 @@ abstract class DnsNameResolverContext<T> {
                 .append(' ');
             }
         }
-        final UnknownHostException cause = new UnknownHostException(buf.toString());
+        final UnknownHostException unknownHostException = new UnknownHostException(buf.toString());
-
+        if (cause == null) {
-        resolveCache.cache(hostname, additionals, cause, parent.ch.eventLoop());
+            // Only cache if the failure was not because of an IO error / timeout that was caused by the query
-        promise.tryFailure(cause);
+            // 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 +759,7 @@ abstract class DnsNameResolverContext<T> {
                 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<T> {
                 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 @@ 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;
 

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 @@ final class DnsQueryContext {
            .append(" (no stack trace available)");
 
         final DnsNameResolverException e;
-        if (cause != null) {
+        if (cause == null) {
-            e = new DnsNameResolverException(nameServerAddr, question(), buf.toString(), cause);
+            // 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 @@ 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<? 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();
+    }
 }