Catch Errors when calling complex parsing code

When we have involved parsing/validation logic, it would be easy to have
bugs. There's little reason we can't handle those bugs, as the logic
shouldn't be modifying global state. While the current config is bad or
trigger a bug, we want to keep working until we get good/working config
again.

For XDS, we do have the problem of losing the exception's stack trace.
We could consider increasing the log level, but we could also consider
propagating the error to the listener. Let's skip figuring out that
exact behavior for now, and just move a step in the right direction.

Author: ejona86

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

@@ -69,8 +69,19 @@ public ConfigOrError parseServiceConfig(Map<String, ?> rawServiceConfig) {
               maxHedgedAttemptsLimit,
               loadBalancingPolicySelection));
     } catch (RuntimeException e) {
+      // TODO(ejona): We really don't want parsers throwing exceptions; they should return an error.
+      // However, right now ManagedChannelServiceConfig itself uses exceptions like
+      // ClassCastException. We should handle those with a graceful return within
+      // ManagedChannelServiceConfig and then get rid of this case. Then all exceptions are
+      // "unexpected" and the INTERNAL status code makes it clear a bug needs to be fixed.
       return ConfigOrError.fromError(
           Status.UNKNOWN.withDescription("failed to parse service config").withCause(e));
+    } catch (Throwable t) {
+      // Even catch Errors, since broken config parsing could trigger AssertionError,
+      // StackOverflowError, and other errors we can reasonably safely recover. Since the config
+      // could be untrusted, we want to error on the side of recovering.
+      return ConfigOrError.fromError(
+          Status.INTERNAL.withDescription("Unexpected error parsing service config").withCause(t));
     }
   }
 }

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

@@ -3942,6 +3942,28 @@ public void nameResolverHelper_badConfigFails() {
     assertThat(coe.getError().getCause()).isInstanceOf(ClassCastException.class);
   }
 
+  @Test
+  public void nameResolverHelper_badParser_failsGracefully() {
+    boolean retryEnabled = false;
+    int maxRetryAttemptsLimit = 2;
+    int maxHedgedAttemptsLimit = 3;
+
+    Throwable t = new Error("really poor config parser");
+    when(mockLoadBalancerProvider.parseLoadBalancingPolicyConfig(any())).thenThrow(t);
+    ScParser parser = new ScParser(
+        retryEnabled,
+        maxRetryAttemptsLimit,
+        maxHedgedAttemptsLimit,
+        mockLoadBalancerProvider);
+
+    ConfigOrError coe = parser.parseServiceConfig(ImmutableMap.of());
+
+    assertThat(coe.getError()).isNotNull();
+    assertThat(coe.getError().getCode()).isEqualTo(Code.INTERNAL);
+    assertThat(coe.getError().getDescription()).contains("Unexpected error parsing service config");
+    assertThat(coe.getError().getCause()).isSameInstanceAs(t);
+  }
+
   @Test
   public void nameResolverHelper_noConfigChosen() {
     boolean retryEnabled = false;

xds/src/main/java/io/grpc/xds/client/XdsResourceType.java

@@ -33,10 +33,14 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10847")
 public abstract class XdsResourceType<T extends ResourceUpdate> {
+  private static final Logger log = Logger.getLogger(XdsResourceType.class.getName());
+
   static final String TYPE_URL_RESOURCE =
       "type.googleapis.com/envoy.service.discovery.v3.Resource";
   protected static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls";
@@ -176,6 +180,16 @@ ValidatedResourceUpdate<T> parse(Args args, List<Any> resources) {
                 typeName(), unpackedClassName().getSimpleName(), cname, e.getMessage()));
         invalidResources.add(cname);
         continue;
+      } catch (Throwable t) {
+        log.log(Level.FINE, "Unexpected error in doParse()", t);
+        String errorMessage = t.getClass().getSimpleName();
+        if (t.getMessage() != null) {
+          errorMessage = errorMessage + ": " + t.getMessage();
+        }
+        errors.add(String.format("%s response '%s' unexpected error: %s",
+                typeName(), cname, errorMessage));
+        invalidResources.add(cname);
+        continue;
       }
 
       // Resource parsed successfully.

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java

@@ -18,6 +18,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static io.grpc.StatusMatcher.statusHasCode;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
@@ -42,6 +43,7 @@
 import com.google.protobuf.Duration;
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.Message;
+import com.google.protobuf.StringValue;
 import com.google.protobuf.UInt32Value;
 import com.google.protobuf.util.Durations;
 import io.envoyproxy.envoy.config.cluster.v3.OutlierDetection;
@@ -59,6 +61,7 @@
 import io.grpc.Status;
 import io.grpc.Status.Code;
 import io.grpc.StatusOr;
+import io.grpc.StatusOrMatcher;
 import io.grpc.inprocess.InProcessChannelBuilder;
 import io.grpc.inprocess.InProcessServerBuilder;
 import io.grpc.internal.BackoffPolicy;
@@ -111,6 +114,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Queue;
 import java.util.concurrent.BlockingDeque;
 import java.util.concurrent.CountDownLatch;
@@ -278,6 +282,8 @@ public long currentTimeNanos() {
   @Mock
   private ResourceWatcher<EdsUpdate> edsResourceWatcher;
   @Mock
+  private ResourceWatcher<StringUpdate> stringResourceWatcher;
+  @Mock
   private XdsClientMetricReporter xdsClientMetricReporter;
   @Mock
   private ServerConnectionCallback serverConnectionCallback;
@@ -667,6 +673,58 @@ private void verifyServerConnection(int times, boolean isConnected, String xdsSe
         eq(xdsServer));
   }
 
+  @Test
+  public void doParse_returnsSuccessfully() {
+    XdsStringResource resourceType = new XdsStringResource();
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasValue(
+        (StringUpdate arg) -> new StringUpdate("resource1").equals(arg))));
+  }
+
+  @Test
+  public void doParse_throwsResourceInvalidException_resourceInvalid() {
+    XdsStringResource resourceType = new XdsStringResource() {
+      @Override
+      protected StringUpdate doParse(Args args, Message unpackedMessage)
+          throws ResourceInvalidException {
+        throw new ResourceInvalidException("some bad input");
+      }
+    };
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasStatus(
+        statusHasCode(Status.Code.UNAVAILABLE)
+          .andDescriptionContains("validation error: some bad input"))));
+  }
+
+  @Test
+  public void doParse_throwsError_resourceInvalid() throws Exception {
+    XdsStringResource resourceType = new XdsStringResource() {
+      @Override
+      protected StringUpdate doParse(Args args, Message unpackedMessage) {
+        throw new AssertionError("something bad happened");
+      }
+    };
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasStatus(
+        statusHasCode(Status.Code.UNAVAILABLE)
+          .andDescriptionContains("unexpected error: AssertionError: something bad happened"))));
+  }
+
   @Test
   public void ldsResourceNotFound() {
     DiscoveryRpcCall call = startResourceWatcher(XdsListenerResource.getInstance(), LDS_RESOURCE,
@@ -5318,4 +5376,70 @@ protected abstract Message buildHttpConnectionManagerFilter(
 
     protected abstract Message buildTerminalFilter();
   }
+
+  private static class XdsStringResource extends XdsResourceType<StringUpdate> {
+    @Override
+    @SuppressWarnings("unchecked")
+    protected Class<? extends com.google.protobuf.Message> unpackedClassName() {
+      return StringValue.class;
+    }
+
+    @Override
+    public String typeName() {
+      return "EMPTY";
+    }
+
+    @Override
+    public String typeUrl() {
+      return "type.googleapis.com/google.protobuf.StringValue";
+    }
+
+    @Override
+    public boolean shouldRetrieveResourceKeysForArgs() {
+      return false;
+    }
+
+    @Override
+    protected boolean isFullStateOfTheWorld() {
+      return false;
+    }
+
+    @Override
+    @Nullable
+    protected String extractResourceName(Message unpackedResource) {
+      if (!(unpackedResource instanceof StringValue)) {
+        return null;
+      }
+      return ((StringValue) unpackedResource).getValue();
+    }
+
+    @Override
+    protected StringUpdate doParse(Args args, Message unpackedMessage)
+        throws ResourceInvalidException {
+      return new StringUpdate(((StringValue) unpackedMessage).getValue());
+    }
+  }
+
+  private static final class StringUpdate implements ResourceUpdate {
+    @SuppressWarnings("UnusedVariable")
+    public final String value;
+
+    public StringUpdate(String value) {
+      this.value = value;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof StringUpdate)) {
+        return false;
+      }
+      StringUpdate that = (StringUpdate) o;
+      return Objects.equals(this.value, that.value);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(value);
+    }
+  }
 }