Revert "core: DnsNameResolver caches refresh (grpc#4812)"

This reverts commit 1899910.

Author: creamsoup

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -20,7 +20,6 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Stopwatch;
 import com.google.common.base.Verify;
 import io.grpc.Attributes;
 import io.grpc.EquivalentAddressGroup;
@@ -43,7 +42,6 @@
 import java.util.Random;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -90,19 +88,6 @@ final class DnsNameResolver extends NameResolver {
   private static final String JNDI_TXT_PROPERTY =
       System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_service_config", "false");
 
-  /**
-   * Java networking system properties name for caching DNS result.
-   *
-   * <p>Default value is -1 (cache forever) if security manager is installed. If security manager is
-   * not installed, the ttl value is {@code null} which falls back to {@link
-   * #DEFAULT_NETWORK_CACHE_TTL_SECONDS gRPC default value}.
-   */
-  @VisibleForTesting
-  static final String NETWORKADDRESS_CACHE_TTL_PROPERTY = "networkaddress.cache.ttl";
-  /** Default DNS cache duration if network cache ttl value is not specified ({@code null}). */
-  @VisibleForTesting
-  static final long DEFAULT_NETWORK_CACHE_TTL_SECONDS = 30;
-
   @VisibleForTesting
   static boolean enableJndi = Boolean.parseBoolean(JNDI_PROPERTY);
   @VisibleForTesting
@@ -130,8 +115,6 @@ final class DnsNameResolver extends NameResolver {
   private final String host;
   private final int port;
   private final Resource<ExecutorService> executorResource;
-  private final long networkAddressCacheTtlNanos;
-  private final Stopwatch stopwatch;
   @GuardedBy("this")
   private boolean shutdown;
   @GuardedBy("this")
@@ -140,11 +123,10 @@ final class DnsNameResolver extends NameResolver {
   private boolean resolving;
   @GuardedBy("this")
   private Listener listener;
-  private ResolutionResults cachedResolutionResults;
 
   DnsNameResolver(@Nullable String nsAuthority, String name, Attributes params,
-      Resource<ExecutorService> executorResource, ProxyDetector proxyDetector,
-      Stopwatch stopwatch) {
+      Resource<ExecutorService> executorResource,
+      ProxyDetector proxyDetector) {
     // TODO: if a DNS server is provided as nsAuthority, use it.
     // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java
     this.executorResource = executorResource;
@@ -167,8 +149,6 @@ final class DnsNameResolver extends NameResolver {
       port = nameUri.getPort();
     }
     this.proxyDetector = proxyDetector;
-    this.stopwatch = Preconditions.checkNotNull(stopwatch, "stopwatch");
-    this.networkAddressCacheTtlNanos = getNetworkAddressCacheTtlNanos();
   }
 
   @Override
@@ -198,13 +178,6 @@ public void run() {
           if (shutdown) {
             return;
           }
-          boolean resourceRefreshRequired = cachedResolutionResults == null
-              || networkAddressCacheTtlNanos == 0
-              || (networkAddressCacheTtlNanos > 0
-                  && stopwatch.elapsed(TimeUnit.NANOSECONDS) > networkAddressCacheTtlNanos);
-          if (!resourceRefreshRequired) {
-            return;
-          }
           savedListener = listener;
           resolving = true;
         }
@@ -234,10 +207,6 @@ public void run() {
             }
             resolutionResults =
                 resolveAll(addressResolver, resourceResolver, enableSrv, enableTxt, host);
-            cachedResolutionResults = resolutionResults;
-            if (networkAddressCacheTtlNanos > 0) {
-              stopwatch.reset().start();
-            }
           } catch (Exception e) {
             savedListener.onError(
                 Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e));
@@ -284,23 +253,6 @@ public void run() {
       }
     };
 
-  /** Returns value of network address cache ttl property. */
-  private static long getNetworkAddressCacheTtlNanos() {
-    String cacheTtlPropertyValue = System.getProperty(NETWORKADDRESS_CACHE_TTL_PROPERTY);
-    long cacheTtl = DEFAULT_NETWORK_CACHE_TTL_SECONDS;
-    if (cacheTtlPropertyValue != null) {
-      try {
-        cacheTtl = Long.parseLong(cacheTtlPropertyValue);
-      } catch (NumberFormatException e) {
-        logger.log(
-            Level.WARNING,
-            "Property({0}) valid is not valid number format({1}), fall back to default({2})",
-            new Object[] {NETWORKADDRESS_CACHE_TTL_PROPERTY, cacheTtlPropertyValue, cacheTtl});
-      }
-    }
-    return cacheTtl > 0 ? TimeUnit.SECONDS.toNanos(cacheTtl) : cacheTtl;
-  }
-
   @GuardedBy("this")
   private void resolve() {
     if (resolving || shutdown) {

core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java

@@ -17,7 +17,6 @@
 package io.grpc.internal;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Stopwatch;
 import io.grpc.Attributes;
 import io.grpc.NameResolverProvider;
 import java.net.URI;
@@ -53,8 +52,7 @@ public DnsNameResolver newNameResolver(URI targetUri, Attributes params) {
           name,
           params,
           GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-          GrpcUtil.getDefaultProxyDetector(),
-          Stopwatch.createUnstarted());
+          GrpcUtil.getDefaultProxyDetector());
     } else {
       return null;
     }

core/src/test/java/io/grpc/internal/DnsNameResolverTest.java

@@ -27,13 +27,10 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
-import com.google.common.base.Stopwatch;
 import com.google.common.collect.Iterables;
 import com.google.common.net.InetAddresses;
-import com.google.common.testing.FakeTicker;
 import io.grpc.Attributes;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.NameResolver;
@@ -55,8 +52,6 @@
 import java.util.Map;
 import java.util.Random;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.TimeUnit;
-import javax.annotation.Nullable;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
@@ -108,45 +103,28 @@ public void close(ExecutorService instance) {
   private NameResolver.Listener mockListener;
   @Captor
   private ArgumentCaptor<List<EquivalentAddressGroup>> resultCaptor;
-  @Nullable
-  private String networkaddressCacheTtlPropertyValue;
 
   private DnsNameResolver newResolver(String name, int port) {
-    return newResolver(name, port, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted());
+    return newResolver(name, port, GrpcUtil.NOOP_PROXY_DETECTOR);
   }
 
   private DnsNameResolver newResolver(
       String name,
       int port,
-      ProxyDetector proxyDetector,
-      Stopwatch stopwatch) {
+      ProxyDetector proxyDetector) {
     DnsNameResolver dnsResolver = new DnsNameResolver(
         null,
         name,
         Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, port).build(),
         fakeExecutorResource,
-        proxyDetector,
-        stopwatch);
+        proxyDetector);
     return dnsResolver;
   }
 
   @Before
   public void setUp() {
     MockitoAnnotations.initMocks(this);
     DnsNameResolver.enableJndi = true;
-    networkaddressCacheTtlPropertyValue =
-        System.getProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY);
-  }
-
-  @After
-  public void restoreSystemProperty() {
-    if (networkaddressCacheTtlPropertyValue == null) {
-      System.clearProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY);
-    } else {
-      System.setProperty(
-          DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY,
-          networkaddressCacheTtlPropertyValue);
-    }
   }
 
   @After
@@ -198,8 +176,7 @@ public void invalidDnsName_containsUnderscore() {
   }
 
   @Test
-  public void resolve_neverCache() throws Exception {
-    System.setProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY, "0");
+  public void resolve() throws Exception {
     final List<InetAddress> answer1 = createAddressList(2);
     final List<InetAddress> answer2 = createAddressList(1);
     String name = "foo.googleapis.com";
@@ -226,156 +203,6 @@ public void resolve_neverCache() throws Exception {
     verify(mockResolver, times(2)).resolveAddress(Matchers.anyString());
   }
 
-  @Test
-  public void resolve_cacheForever() throws Exception {
-    System.setProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY, "-1");
-    final List<InetAddress> answer1 = createAddressList(2);
-    String name = "foo.googleapis.com";
-    FakeTicker fakeTicker = new FakeTicker();
-
-    DnsNameResolver resolver =
-        newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker));
-    AddressResolver mockResolver = mock(AddressResolver.class);
-    when(mockResolver.resolveAddress(Matchers.anyString()))
-        .thenReturn(answer1)
-        .thenThrow(new AssertionError("should not called twice"));
-    resolver.setAddressResolver(mockResolver);
-
-    resolver.start(mockListener);
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer1, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    fakeTicker.advance(1, TimeUnit.DAYS);
-    resolver.refresh();
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verifyNoMoreInteractions(mockListener);
-    assertAnswerMatches(answer1, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    resolver.shutdown();
-
-    verify(mockResolver).resolveAddress(Matchers.anyString());
-  }
-
-  @Test
-  public void resolve_usingCache() throws Exception {
-    long ttl = 60;
-    System.setProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY, Long.toString(ttl));
-    final List<InetAddress> answer = createAddressList(2);
-    String name = "foo.googleapis.com";
-    FakeTicker fakeTicker = new FakeTicker();
-
-    DnsNameResolver resolver =
-        newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker));
-    AddressResolver mockResolver = mock(AddressResolver.class);
-    when(mockResolver.resolveAddress(Matchers.anyString()))
-        .thenReturn(answer)
-        .thenThrow(new AssertionError("should not reach here."));
-    resolver.setAddressResolver(mockResolver);
-
-    resolver.start(mockListener);
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    // this refresh should return cached result
-    fakeTicker.advance(ttl - 1, TimeUnit.SECONDS);
-    resolver.refresh();
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verifyNoMoreInteractions(mockListener);
-    assertAnswerMatches(answer, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    resolver.shutdown();
-
-    verify(mockResolver).resolveAddress(Matchers.anyString());
-  }
-
-  @Test
-  public void resolve_cacheExpired() throws Exception {
-    long ttl = 60;
-    System.setProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY, Long.toString(ttl));
-    final List<InetAddress> answer1 = createAddressList(2);
-    final List<InetAddress> answer2 = createAddressList(1);
-    String name = "foo.googleapis.com";
-    FakeTicker fakeTicker = new FakeTicker();
-
-    DnsNameResolver resolver =
-        newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker));
-    AddressResolver mockResolver = mock(AddressResolver.class);
-    when(mockResolver.resolveAddress(Matchers.anyString())).thenReturn(answer1).thenReturn(answer2);
-    resolver.setAddressResolver(mockResolver);
-
-    resolver.start(mockListener);
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer1, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    fakeTicker.advance(ttl + 1, TimeUnit.SECONDS);
-    resolver.refresh();
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener, times(2)).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer2, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    resolver.shutdown();
-
-    verify(mockResolver, times(2)).resolveAddress(Matchers.anyString());
-  }
-
-  @Test
-  public void resolve_invalidTtlPropertyValue() throws Exception {
-    System.setProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY, "not_a_number");
-    resolveDefaultValue();
-  }
-
-  @Test
-  public void resolve_noPropertyValue() throws Exception {
-    System.clearProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY);
-    resolveDefaultValue();
-  }
-
-  private void resolveDefaultValue() throws Exception {
-    final List<InetAddress> answer1 = createAddressList(2);
-    final List<InetAddress> answer2 = createAddressList(1);
-    String name = "foo.googleapis.com";
-    FakeTicker fakeTicker = new FakeTicker();
-
-    DnsNameResolver resolver =
-        newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker));
-    AddressResolver mockResolver = mock(AddressResolver.class);
-    when(mockResolver.resolveAddress(Matchers.anyString())).thenReturn(answer1).thenReturn(answer2);
-    resolver.setAddressResolver(mockResolver);
-
-    resolver.start(mockListener);
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer1, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    fakeTicker.advance(DnsNameResolver.DEFAULT_NETWORK_CACHE_TTL_SECONDS, TimeUnit.SECONDS);
-    resolver.refresh();
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verifyNoMoreInteractions(mockListener);
-    assertAnswerMatches(answer1, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    fakeTicker.advance(1, TimeUnit.SECONDS);
-    resolver.refresh();
-    assertEquals(1, fakeExecutor.runDueTasks());
-    verify(mockListener, times(2)).onAddresses(resultCaptor.capture(), any(Attributes.class));
-    assertAnswerMatches(answer2, 81, resultCaptor.getValue());
-    assertEquals(0, fakeClock.numPendingTasks());
-
-    resolver.shutdown();
-
-    verify(mockResolver, times(2)).resolveAddress(Matchers.anyString());
-  }
-
   @Test
   public void resolveAll_nullResourceResolver() throws Exception {
     final String hostname = "addr.fake";
@@ -502,8 +329,7 @@ public void doNotResolveWhenProxyDetected() throws Exception {
         "password");
     when(alwaysDetectProxy.proxyFor(any(SocketAddress.class)))
         .thenReturn(proxyParameters);
-    DnsNameResolver resolver =
-        newResolver(name, port, alwaysDetectProxy, Stopwatch.createUnstarted());
+    DnsNameResolver resolver = newResolver(name, port, alwaysDetectProxy);
     AddressResolver mockAddressResolver = mock(AddressResolver.class);
     when(mockAddressResolver.resolveAddress(Matchers.anyString())).thenThrow(new AssertionError());
     resolver.setAddressResolver(mockAddressResolver);