Skip to content

Commit

Permalink
Remove DnsClient workaround for Netty SRV cache issue (#1257)
Browse files Browse the repository at this point in the history
Motivation:
Netty used to use the local DNS cache for SRV record resolution, but
this lead to false positive errors in some situations due to partial
caching. However that issue has been fixed [1] and included in the latest
release.

[1] netty/netty#10808

Modifications:
- Remove the DnsNameResolver that was used specifically to disable the
DNS cache.

Result:
Removal of workaround code in DnsClient with no change in behavior.
  • Loading branch information
Scottmitch committed Dec 9, 2020
1 parent 17be71a commit e7d36f3
Showing 1 changed file with 1 addition and 12 deletions.
Expand Up @@ -45,7 +45,6 @@
import io.netty.resolver.dns.DefaultDnsCache;
import io.netty.resolver.dns.DnsNameResolver;
import io.netty.resolver.dns.DnsNameResolverBuilder;
import io.netty.resolver.dns.NoopDnsCnameCache;
import io.netty.util.ReferenceCountUtil;
import io.netty.util.concurrent.Future;
import io.netty.util.concurrent.FutureListener;
Expand Down Expand Up @@ -108,7 +107,6 @@ final class DefaultDnsClient implements DnsClient {

private final EventLoopAwareNettyIoExecutor nettyIoExecutor;
private final DnsNameResolver resolver;
private final DnsNameResolver srvResolver;
private final MinTtlCache ttlCache;
private final ListenableAsyncCloseable asyncCloseable;
@Nullable
Expand Down Expand Up @@ -180,14 +178,6 @@ final class DefaultDnsClient implements DnsClient {
}

resolver = builder.build();

// TODO(scott): SRV records aren't cached, this may result in unexpected failures for SRV queries if the
// hostname has CNAME entries which map to SRV entries with lower TTLs. We should be able to use the same
// resolver for A* and SRV records after this is resolved.
// See DefaultDnsClientTest#srvWithCNAMEEntryLowerTTLDoesNotFail.
// See https://github.com/netty/netty/pull/10808
builder.cnameCache(NoopDnsCnameCache.INSTANCE);
srvResolver = builder.build();
}

@Nullable
Expand Down Expand Up @@ -291,7 +281,6 @@ private void closeAsync0() {
}
closed = true;
resolver.close();
srvResolver.close();
ttlCache.clear();
}

Expand All @@ -316,7 +305,7 @@ protected AbstractDnsSubscription newSubscription(
@Override
protected Future<DnsAnswer<HostAndPort>> doDnsQuery() {
Promise<DnsAnswer<HostAndPort>> promise = ImmediateEventExecutor.INSTANCE.newPromise();
srvResolver.resolveAll(new DefaultDnsQuestion(name, SRV))
resolver.resolveAll(new DefaultDnsQuestion(name, SRV))
.addListener((Future<? super List<DnsRecord>> completedFuture) -> {
Throwable cause = completedFuture.cause();
if (cause != null) {
Expand Down

0 comments on commit e7d36f3

Please sign in to comment.