Remove InternalResolutionResult from DnsNameResolver

The internal result was needed before 90d0fab allowed addresses to
fail yet still provide attributes and service config. Now the code can
just use the regular API.

This does cause a behavior change where TXT records are looked up even
if address lookup failed, however that's actually what we wanted to
allow in 90d0fab by adding the new API. Also, the TXT code was added
in 2017 and it's now 2026 yet it is still disabled, so it's unlikely to
matter soon.

Author: ejona86

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

@@ -25,7 +25,6 @@
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Verify;
 import com.google.common.base.VerifyException;
-import io.grpc.Attributes;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.NameResolver;
 import io.grpc.ProxiedSocketAddress;
@@ -262,22 +261,19 @@ private EquivalentAddressGroup detectProxy() throws IOException {
   /**
    * Main logic of name resolution.
    */
-  protected InternalResolutionResult doResolve(boolean forceTxt) {
-    InternalResolutionResult result = new InternalResolutionResult();
+  protected ResolutionResult doResolve() {
+    ResolutionResult.Builder resultBuilder = ResolutionResult.newBuilder();
     try {
-      result.addresses = resolveAddresses();
+      resultBuilder.setAddressesOrError(StatusOr.fromValue(resolveAddresses()));
     } catch (Exception e) {
       logger.log(Level.FINE, "Address resolution failure", e);
-      if (!forceTxt) {
-        result.error =
-            Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e);
-        return result;
-      }
+      resultBuilder.setAddressesOrError(StatusOr.fromStatus(
+          Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e)));
     }
     if (enableTxt) {
-      result.config = resolveServiceConfig();
+      resultBuilder.setServiceConfig(resolveServiceConfig());
     }
-    return result;
+    return resultBuilder.build();
   }
 
   private final class Resolve implements Runnable {
@@ -292,38 +288,22 @@ public void run() {
       if (logger.isLoggable(Level.FINER)) {
         logger.finer("Attempting DNS resolution of " + host);
       }
-      InternalResolutionResult result = null;
+      ResolutionResult result = null;
       try {
         EquivalentAddressGroup proxiedAddr = detectProxy();
-        ResolutionResult.Builder resolutionResultBuilder = ResolutionResult.newBuilder();
         if (proxiedAddr != null) {
           if (logger.isLoggable(Level.FINER)) {
             logger.finer("Using proxy address " + proxiedAddr);
           }
-          resolutionResultBuilder.setAddressesOrError(
-              StatusOr.fromValue(Collections.singletonList(proxiedAddr)));
+          result = ResolutionResult.newBuilder()
+              .setAddressesOrError(StatusOr.fromValue(Collections.singletonList(proxiedAddr)))
+              .build();
         } else {
-          result = doResolve(false);
-          if (result.error != null) {
-            InternalResolutionResult finalResult = result;
-            syncContext.execute(() ->
-                savedListener.onResult2(ResolutionResult.newBuilder()
-                    .setAddressesOrError(StatusOr.fromStatus(finalResult.error))
-                    .build()));
-            return;
-          }
-          if (result.addresses != null) {
-            resolutionResultBuilder.setAddressesOrError(StatusOr.fromValue(result.addresses));
-          }
-          if (result.config != null) {
-            resolutionResultBuilder.setServiceConfig(result.config);
-          }
-          if (result.attributes != null) {
-            resolutionResultBuilder.setAttributes(result.attributes);
-          }
+          result = doResolve();
         }
+        ResolutionResult savedResult = result;
         syncContext.execute(() -> {
-          savedListener.onResult2(resolutionResultBuilder.build());
+          savedListener.onResult2(savedResult);
         });
       } catch (IOException e) {
         syncContext.execute(() ->
@@ -333,7 +313,7 @@ public void run() {
                         Status.UNAVAILABLE.withDescription(
                             "Unable to resolve host " + host).withCause(e))).build()));
       } finally {
-        final boolean succeed = result != null && result.error == null;
+        final boolean succeed = result != null && result.getAddressesOrError().hasValue();
         syncContext.execute(new Runnable() {
           @Override
           public void run() {
@@ -535,18 +515,6 @@ private static long getNetworkAddressCacheTtlNanos(boolean isAndroid) {
     return sc;
   }
 
-  /**
-   * Used as a DNS-based name resolver's internal representation of resolution result.
-   */
-  protected static final class InternalResolutionResult {
-    private Status error;
-    private List<EquivalentAddressGroup> addresses;
-    private ConfigOrError config;
-    public Attributes attributes;
-
-    private InternalResolutionResult() {}
-  }
-
   /**
    * Describes a parsed SRV record.
    */

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

@@ -684,7 +684,7 @@ public ConfigOrError parseServiceConfig(Map<String, ?> rawServiceConfig) {
   }
 
   @Test
-  public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception {
+  public void resolve_addressFailure_stillLookUpServiceConfig() throws Exception {
     DnsNameResolver.enableTxt = true;
     AddressResolver mockAddressResolver = mock(AddressResolver.class);
     when(mockAddressResolver.resolveAddress(anyString()))
@@ -703,7 +703,7 @@ public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception {
     Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus();
     assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE);
     assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr");
-    verify(mockResourceResolver, never()).resolveTxt(anyString());
+    verify(mockResourceResolver).resolveTxt("_grpc_config." + name);
 
     assertEquals(0, fakeClock.numPendingTasks());
     // A retry should be scheduled

grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java

@@ -21,6 +21,7 @@
 import io.grpc.Attributes;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.NameResolver;
+import io.grpc.StatusOr;
 import io.grpc.internal.DnsNameResolver;
 import io.grpc.internal.SharedResourceHolder.Resource;
 import java.net.InetAddress;
@@ -58,14 +59,22 @@ final class GrpclbNameResolver extends DnsNameResolver {
   }
 
   @Override
-  protected InternalResolutionResult doResolve(boolean forceTxt) {
+  protected ResolutionResult doResolve() {
+    ResolutionResult result = super.doResolve();
     List<EquivalentAddressGroup> balancerAddrs = resolveBalancerAddresses();
-    InternalResolutionResult result = super.doResolve(!balancerAddrs.isEmpty());
     if (!balancerAddrs.isEmpty()) {
-      result.attributes =
-          Attributes.newBuilder()
+      ResolutionResult.Builder resultBuilder = result.toBuilder()
+          .setAttributes(result.getAttributes().toBuilder()
               .set(GrpclbConstants.ATTR_LB_ADDRS, balancerAddrs)
-              .build();
+              .build());
+      if (!result.getAddressesOrError().hasValue()) {
+        // While ResolutionResult is powerful enough to communicate attributes simultaneously with
+        // an address resolution failure, LoadBalancer.ResolvedAddresses isn't yet and so the
+        // attributes are lost if addresses fail. GrpclbLB will be able to handle the lack of
+        // addresses since there are LB addresses, so discard the failure for now.
+        resultBuilder.setAddressesOrError(StatusOr.fromValue(Collections.emptyList()));
+      }
+      result = resultBuilder.build();
     }
     return result;
   }

grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java

@@ -20,7 +20,6 @@
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -319,7 +318,7 @@ public void resolveAll_balancerLookupFails_stillLookUpServiceConfig() throws Exc
   }
 
   @Test
-  public void resolve_addressAndBalancersLookupFail_neverLookupServiceConfig() throws Exception {
+  public void resolve_addressAndBalancersLookupFail_stillLookupServiceConfig() throws Exception {
     AddressResolver mockAddressResolver = mock(AddressResolver.class);
     when(mockAddressResolver.resolveAddress(anyString()))
         .thenThrow(new UnknownHostException("I really tried"));
@@ -338,7 +337,7 @@ public void resolve_addressAndBalancersLookupFail_neverLookupServiceConfig() thr
     Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus();
     assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE);
     verify(mockAddressResolver).resolveAddress(hostName);
-    verify(mockResourceResolver, never()).resolveTxt("_grpc_config." + hostName);
+    verify(mockResourceResolver).resolveTxt("_grpc_config." + hostName);
     verify(mockResourceResolver).resolveSrv("_grpclb._tcp." + hostName);
   }
 }