Fix another resource leak in DnsNameResolver

- Fix a bug in cache expiration task; wrong object was being released - Added more sanity checks when caching an entry
sunng87 · Oct 17, 2014 · 29a8475 · 29a8475
1 parent e809f97
commit 29a8475
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 11 deletions.
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
@@ -726,14 +726,17 @@ private Future<DnsResponse> query0(
     }
 
     void cache(final DnsQuestion question, DnsCacheEntry entry, long delaySeconds) {
-        queryCache.put(question, entry);
+        DnsCacheEntry oldEntry = queryCache.put(question, entry);
+        if (oldEntry != null) {
+            oldEntry.release();
+        }
+
         boolean scheduled = false;
         try {
             entry.expirationFuture = ch.eventLoop().schedule(new OneTimeTask() {
                 @Override
                 public void run() {
-                    Object response = queryCache.remove(question);
-                    ReferenceCountUtil.safeRelease(response);
+                    clearCache(question);
                 }
             }, delaySeconds, TimeUnit.SECONDS);
 
@@ -742,7 +745,7 @@ public void run() {
             if (!scheduled) {
                 // If failed to schedule the expiration task,
                 // remove the entry from the cache so that it does not leak.
-                queryCache.remove(question);
+                clearCache(question);
                 entry.release();
             }
         }
@@ -789,12 +792,16 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
                 if (res.header().responseCode() == DnsResponseCode.NOERROR) {
                     cache(q, res);
                     promises.set(queryId, null);
-                    qCtx.promise().trySuccess(res.retain());
+
+                    Promise<DnsResponse> qPromise = qCtx.promise();
+                    if (qPromise.setUncancellable()) {
+                        qPromise.setSuccess(res.retain());
+                    }
                 } else {
                     qCtx.retry(res.sender(),
-                            "response code: " + res.header().responseCode() +
-                            " with " + res.answers().size() + " answer(s) and " +
-                            res.authorityResources().size() + " authority resource(s)");
+                               "response code: " + res.header().responseCode() +
+                               " with " + res.answers().size() + " answer(s) and " +
+                               res.authorityResources().size() + " authority resource(s)");
                 }
             } finally {
                 ReferenceCountUtil.safeRelease(msg);

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
@@ -30,8 +30,8 @@
 import io.netty.util.internal.ThreadLocalRandom;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
+import org.junit.After;
 import org.junit.AfterClass;
-import org.junit.Before;
 import org.junit.Test;
 
 import java.net.Inet4Address;
@@ -253,8 +253,8 @@ public static void destroy() {
         group.shutdownGracefully();
     }
 
-    @Before
-    public void reset() {
+    @After
+    public void reset() throws Exception {
         resolver.clearCache();
     }