From d1b99b702c3cd76d2b8ed277274b7f18c04db104 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 18 May 2020 14:28:30 +0200 Subject: [PATCH] Use allocation free algorithm to detect CNAME cache loops (#10291) Motivation: We did use a HashSet to detect CNAME cache loops which needs allocations. We can use an algorithm that doesnt need any allocations Modifications: Use algorithm that doesnt need allocations Result: Less allocations on the slow path --- .../netty/resolver/dns/DnsResolveContext.java | 26 +++++----- .../resolver/dns/DnsResolveContextTest.java | 48 +++++++++++++++++++ 2 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 resolver-dns/src/test/java/io/netty/resolver/dns/DnsResolveContextTest.java 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 091ec4d6da..3ba521b2c2 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 @@ -49,7 +49,6 @@ import java.util.AbstractList; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.IdentityHashMap; import java.util.Iterator; import java.util.List; @@ -278,8 +277,9 @@ abstract class DnsResolveContext { // Resolve the final name from the CNAME cache until there is nothing to follow anymore. This also // guards against loops in the cache but early return once a loop is detected. - private String cnameResolveFromCache(String name) { - DnsCnameCache cnameCache = cnameCache(); + // + // Visible for testing only + static String cnameResolveFromCache(DnsCnameCache cnameCache, String name) { String first = cnameCache.get(hostnameWithDot(name)); if (first == null) { // Nothing in the cache at all @@ -300,28 +300,30 @@ abstract class DnsResolveContext { return cnameResolveFromCacheLoop(cnameCache, first, second); } - private String cnameResolveFromCacheLoop(DnsCnameCache cnameCache, String first, String mapping) { - // Detect loops using a HashSet. We use this as last resort implementation to reduce allocations in the most - // common cases. - Set cnames = new HashSet(4); - cnames.add(first); - cnames.add(mapping); + static String cnameResolveFromCacheLoop(DnsCnameCache cnameCache, String first, String mapping) { + // Detect loops by advance only every other iteration. + // See https://en.wikipedia.org/wiki/Cycle_detection#Floyd's_Tortoise_and_Hare + boolean advance = false; String name = mapping; // Resolve from cnameCache() until there is no more cname entry cached. while ((mapping = cnameCache.get(hostnameWithDot(name))) != null) { - if (!cnames.add(mapping)) { + if (first.equals(mapping)) { // Follow CNAME from cache would loop. Lets break here. break; } name = mapping; + if (advance) { + first = cnameCache.get(first); + } + advance = !advance; } return name; } private void internalResolve(String name, Promise> promise) { // Resolve from cnameCache() until there is no more cname entry cached. - name = cnameResolveFromCache(name); + name = cnameResolveFromCache(cnameCache(), name); try { DnsServerAddressStream nameServerAddressStream = getNameServers(name); @@ -993,7 +995,7 @@ abstract class DnsResolveContext { private void followCname(DnsQuestion question, String cname, DnsQueryLifecycleObserver queryLifecycleObserver, Promise> promise) { - cname = cnameResolveFromCache(cname); + cname = cnameResolveFromCache(cnameCache(), cname); DnsServerAddressStream stream = getNameServers(cname); final DnsQuestion cnameQuestion; diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsResolveContextTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsResolveContextTest.java new file mode 100644 index 0000000000..70c2cce4e5 --- /dev/null +++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsResolveContextTest.java @@ -0,0 +1,48 @@ +/* + * Copyright 2020 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.channel.embedded.EmbeddedChannel; +import org.junit.Test; + +public class DnsResolveContextTest { + + private static final String HOSTNAME = "netty.io."; + + @Test + public void testCnameLoop() { + for (int i = 1; i < 128; i++) { + DnsResolveContext.cnameResolveFromCache(buildCache(i), HOSTNAME); + } + } + + private static DnsCnameCache buildCache(int chainLength) { + EmbeddedChannel channel = new EmbeddedChannel(); + DnsCnameCache cache = new DefaultDnsCnameCache(); + if (chainLength == 1) { + cache.cache(HOSTNAME, HOSTNAME, Long.MAX_VALUE, channel.eventLoop()); + } else { + String lastName = HOSTNAME; + for (int i = 1; i < chainLength; i++) { + String nextName = i + "." + lastName; + cache.cache(lastName, nextName, Long.MAX_VALUE, channel.eventLoop()); + lastName = nextName; + } + cache.cache(lastName, HOSTNAME, Long.MAX_VALUE, channel.eventLoop()); + } + return cache; + } +}