fix: update retry handling to retry idempotent requests that encounter unexpected EOF while parsing json responses

google-cloud-storage/pom.xml

@@ -87,6 +87,11 @@
       <groupId>org.threeten</groupId>
       <artifactId>threetenbp</artifactId>
     </dependency>
+    <!-- Access to exception for retry handling -->
+    <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-core</artifactId>
+    </dependency>
 
     <!-- Test dependencies -->
     <dependency>

google-cloud-storage/src/main/java/com/google/cloud/storage/DefaultStorageRetryStrategy.java

@@ -16,11 +16,13 @@
 
 package com.google.cloud.storage;
 
+import com.fasterxml.jackson.core.io.JsonEOFException;
 import com.google.api.client.http.HttpResponseException;
 import com.google.cloud.BaseServiceException;
 import com.google.cloud.ExceptionHandler;
 import com.google.cloud.ExceptionHandler.Interceptor;
 import com.google.common.collect.ImmutableSet;
+import com.google.gson.stream.MalformedJsonException;
 import java.io.IOException;
 import java.util.Set;
 
@@ -33,7 +35,8 @@ final class DefaultStorageRetryStrategy implements StorageRetryStrategy {
   private static final Interceptor INTERCEPTOR_NON_IDEMPOTENT =
       new InterceptorImpl(false, ImmutableSet.of());
 
-  private static final ExceptionHandler IDEMPOTENT_HANDLER = newHandler(INTERCEPTOR_IDEMPOTENT);
+  private static final ExceptionHandler IDEMPOTENT_HANDLER =
+      newHandler(new EmptyJsonParsingExceptionInterceptor(), INTERCEPTOR_IDEMPOTENT);
   private static final ExceptionHandler NON_IDEMPOTENT_HANDLER =
       newHandler(INTERCEPTOR_NON_IDEMPOTENT);
 
@@ -47,15 +50,11 @@ public ExceptionHandler getNonidempotentHandler() {
     return NON_IDEMPOTENT_HANDLER;
   }
 
-  private static ExceptionHandler newHandler(Interceptor interceptor) {
-    return ExceptionHandler.newBuilder()
-        .retryOn(BaseServiceException.class)
-        .retryOn(IOException.class)
-        .addInterceptors(interceptor)
-        .build();
+  private static ExceptionHandler newHandler(Interceptor... interceptors) {
+    return ExceptionHandler.newBuilder().addInterceptors(interceptors).build();
   }
 
-  private static class InterceptorImpl implements Interceptor {
+  private static class InterceptorImpl implements BaseInterceptor {
 
     private static final long serialVersionUID = -5153236691367895096L;
     private final boolean idempotent;
@@ -66,11 +65,6 @@ private InterceptorImpl(boolean idempotent, Set<BaseServiceException.Error> retr
       this.retryableErrors = ImmutableSet.copyOf(retryableErrors);
     }
 
-    @Override
-    public RetryResult afterEval(Exception exception, RetryResult retryResult) {
-      return RetryResult.CONTINUE_EVALUATION;
-    }
-
     @Override
     public RetryResult beforeEval(Exception exception) {
       if (exception instanceof BaseServiceException) {
@@ -95,7 +89,11 @@ private RetryResult shouldRetryCodeReason(Integer code, String reason) {
     }
 
     private RetryResult shouldRetryIOException(IOException ioException) {
-      if (BaseServiceException.isRetryable(idempotent, ioException)) {
+      if (ioException instanceof JsonEOFException && idempotent) { // Jackson
+        return RetryResult.RETRY;
+      } else if (ioException instanceof MalformedJsonException && idempotent) { // Gson
+        return RetryResult.RETRY;
+      } else if (BaseServiceException.isRetryable(idempotent, ioException)) {
         return RetryResult.RETRY;
       } else {
         return RetryResult.NO_RETRY;
@@ -117,4 +115,26 @@ private RetryResult deepShouldRetry(BaseServiceException baseServiceException) {
       return shouldRetryCodeReason(code, reason);
     }
   }
+
+  private static final class EmptyJsonParsingExceptionInterceptor implements BaseInterceptor {
+    private static final long serialVersionUID = -3320984020388043628L;
+
+    @Override
+    public RetryResult beforeEval(Exception exception) {
+      if (exception instanceof IllegalArgumentException) {
+        IllegalArgumentException illegalArgumentException = (IllegalArgumentException) exception;
+        if (illegalArgumentException.getMessage().equals("no JSON input found")) {
+          return RetryResult.RETRY;
+        }
+      }
+      return RetryResult.CONTINUE_EVALUATION;
+    }
+  }
+
+  private interface BaseInterceptor extends Interceptor {
+    @Override
+    default RetryResult afterEval(Exception exception, RetryResult retryResult) {
+      return RetryResult.CONTINUE_EVALUATION;
+    }
+  }
 }

google-cloud-storage/src/test/java/com/google/cloud/storage/DataGeneration.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright 2021 Google LLC
+ *
+ * Licensed 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 com.google.cloud.storage;
+
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.Random;
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.Statement;
+
+public final class DataGeneration implements TestRule {
+
+  private final Random rand;
+
+  public DataGeneration(Random rand) {
+    this.rand = rand;
+  }
+
+  public ByteBuffer randByteBuffer(int limit) {
+    ByteBuffer b = ByteBuffer.allocate(limit);
+    fillByteBuffer(b);
+    return b;
+  }
+
+  public void fillByteBuffer(ByteBuffer b) {
+    while (b.position() < b.limit()) {
+      int i = rand.nextInt('z');
+      char c = (char) i;
+      if (Character.isLetter(c) || Character.isDigit(c)) {
+        b.put(Character.toString(c).getBytes(StandardCharsets.UTF_8));
+      }
+    }
+    b.position(0);
+  }
+
+  @Override
+  public Statement apply(Statement base, Description description) {
+    return new Statement() {
+      @Override
+      public void evaluate() throws Throwable {
+        base.evaluate();
+      }
+    };
+  }
+}

google-cloud-storage/src/test/java/com/google/cloud/storage/DefaultRetryHandlingBehaviorTest.java

@@ -20,13 +20,16 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertWithMessage;
 
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.core.io.JsonEOFException;
 import com.google.api.client.googleapis.json.GoogleJsonError;
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.client.http.HttpResponseException;
 import com.google.api.gax.retrying.ResultRetryAlgorithm;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.google.gson.stream.MalformedJsonException;
 import java.io.IOException;
 import java.net.SocketException;
 import java.net.SocketTimeoutException;
@@ -319,6 +322,14 @@ enum ThrowableCategory {
             "connectionClosedPrematurely",
             "connectionClosedPrematurely",
             C.CONNECTION_CLOSED_PREMATURELY)),
+    EMPTY_JSON_PARSE_ERROR(new IllegalArgumentException("no JSON input found")),
+    JACKSON_EOF_EXCEPTION(C.JACKSON_EOF_EXCEPTION),
+    STORAGE_EXCEPTION_0_JACKSON_EOF_EXCEPTION(
+        new StorageException(0, "parse error", C.JACKSON_EOF_EXCEPTION)),
+    GSON_MALFORMED_EXCEPTION(C.GSON_MALFORMED_EXCEPTION),
+    STORAGE_EXCEPTION_0_GSON_MALFORMED_EXCEPTION(
+        new StorageException(0, "parse error", C.GSON_MALFORMED_EXCEPTION)),
+    IO_EXCEPTION(new IOException("no retry")),
     ;
 
     private final Throwable throwable;
@@ -385,6 +396,10 @@ private static final class C {
           new IllegalArgumentException("illegal argument");
       private static final IOException CONNECTION_CLOSED_PREMATURELY =
           new IOException("simulated Connection closed prematurely");
+      private static final JsonEOFException JACKSON_EOF_EXCEPTION =
+          new JsonEOFException(null, JsonToken.VALUE_STRING, "parse-exception");
+      private static final MalformedJsonException GSON_MALFORMED_EXCEPTION =
+          new MalformedJsonException("parse-exception");
 
       private static HttpResponseException newHttpResponseException(
           int httpStatusCode, String name) {
@@ -913,7 +928,67 @@ private static ImmutableList<Case> getAllCases() {
                 ThrowableCategory.STORAGE_EXCEPTION_0_INTERNAL_ERROR,
                 HandlerCategory.NONIDEMPOTENT,
                 ExpectRetry.NO,
-                Behavior.DEFAULT_MORE_STRICT))
+                Behavior.DEFAULT_MORE_STRICT),
+            new Case(
+                ThrowableCategory.EMPTY_JSON_PARSE_ERROR,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.YES,
+                Behavior.DEFAULT_MORE_PERMISSIBLE),
+            new Case(
+                ThrowableCategory.EMPTY_JSON_PARSE_ERROR,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.IO_EXCEPTION,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.IO_EXCEPTION,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.JACKSON_EOF_EXCEPTION,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.YES,
+                Behavior.DEFAULT_MORE_PERMISSIBLE),
+            new Case(
+                ThrowableCategory.JACKSON_EOF_EXCEPTION,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.STORAGE_EXCEPTION_0_JACKSON_EOF_EXCEPTION,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.YES,
+                Behavior.DEFAULT_MORE_PERMISSIBLE),
+            new Case(
+                ThrowableCategory.STORAGE_EXCEPTION_0_JACKSON_EOF_EXCEPTION,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.GSON_MALFORMED_EXCEPTION,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.YES,
+                Behavior.DEFAULT_MORE_PERMISSIBLE),
+            new Case(
+                ThrowableCategory.GSON_MALFORMED_EXCEPTION,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME),
+            new Case(
+                ThrowableCategory.STORAGE_EXCEPTION_0_GSON_MALFORMED_EXCEPTION,
+                HandlerCategory.IDEMPOTENT,
+                ExpectRetry.YES,
+                Behavior.DEFAULT_MORE_PERMISSIBLE),
+            new Case(
+                ThrowableCategory.STORAGE_EXCEPTION_0_GSON_MALFORMED_EXCEPTION,
+                HandlerCategory.NONIDEMPOTENT,
+                ExpectRetry.NO,
+                Behavior.SAME))
         .build();
   }
 }

google-cloud-storage/src/test/java/com/google/cloud/storage/PackagePrivateMethodWorkarounds.java

@@ -16,7 +16,11 @@
 
 package com.google.cloud.storage;
 
+import com.google.api.services.storage.model.StorageObject;
+import com.google.cloud.WriteChannel;
 import com.google.cloud.storage.BucketInfo.BuilderImpl;
+import java.util.Optional;
+import java.util.function.Function;
 
 /**
  * Several classes in the High Level Model for storage include package-local constructors and
@@ -38,4 +42,15 @@ public static Blob blobCopyWithStorage(Blob b, Storage s) {
     BlobInfo.BuilderImpl builder = (BlobInfo.BuilderImpl) BlobInfo.fromPb(b.toPb()).toBuilder();
     return new Blob(s, builder);
   }
+
+  public static Function<WriteChannel, Optional<StorageObject>> maybeGetStorageObjectFunction() {
+    return (w) -> {
+      if (w instanceof BlobWriteChannel) {
+        BlobWriteChannel blobWriteChannel = (BlobWriteChannel) w;
+        return Optional.of(blobWriteChannel.getStorageObject());
+      } else {
+        return Optional.empty();
+      }
+    };
+  }
 }

google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CleanupStrategy.java

@@ -16,7 +16,7 @@
 
 package com.google.cloud.storage.conformance.retry;
 
-enum CleanupStrategy {
+public enum CleanupStrategy {
   ALWAYS,
   ONLY_ON_SUCCESS,
   NEVER

google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RetryTestFixture.java

@@ -21,15 +21,10 @@
 import com.google.api.gax.retrying.RetrySettings;
 import com.google.api.gax.rpc.FixedHeaderProvider;
 import com.google.cloud.NoCredentials;
-import com.google.cloud.conformance.storage.v1.InstructionList;
-import com.google.cloud.conformance.storage.v1.Method;
 import com.google.cloud.storage.Storage;
 import com.google.cloud.storage.StorageOptions;
 import com.google.cloud.storage.conformance.retry.TestBench.RetryTestResource;
 import com.google.common.collect.ImmutableMap;
-import com.google.gson.JsonArray;
-import com.google.gson.JsonObject;
-import java.util.Map;
 import java.util.logging.Logger;
 import org.junit.AssumptionViolatedException;
 import org.junit.rules.TestRule;
@@ -88,7 +83,7 @@ public void evaluate() throws Throwable {
         try {
           LOGGER.fine("Setting up retry_test resource...");
           RetryTestResource retryTestResource =
-              newRetryTestResource(
+              RetryTestResource.newRetryTestResource(
                   testRetryConformance.getMethod(), testRetryConformance.getInstruction());
           retryTest = testBench.createRetryTest(retryTestResource);
           LOGGER.fine("Setting up retry_test resource complete");
@@ -123,17 +118,6 @@ private boolean shouldCleanup(boolean testSuccess, boolean testSkipped) {
         || ((testSuccess || testSkipped) && cleanupStrategy == CleanupStrategy.ONLY_ON_SUCCESS);
   }
 
-  private static RetryTestResource newRetryTestResource(Method m, InstructionList l) {
-    RetryTestResource resource = new RetryTestResource();
-    resource.instructions = new JsonObject();
-    JsonArray instructions = new JsonArray();
-    for (String s : l.getInstructionsList()) {
-      instructions.add(s);
-    }
-    resource.instructions.add(m.getName(), instructions);
-    return resource;
-  }
-
   private Storage newStorage(boolean forTest) {
     StorageOptions.Builder builder =
         StorageOptions.newBuilder()
@@ -145,23 +129,14 @@ private Storage newStorage(boolean forTest) {
     if (forTest) {
       builder
           .setHeaderProvider(
-              new FixedHeaderProvider() {
-                @Override
-                public Map<String, String> getHeaders() {
-                  return ImmutableMap.of(
-                      "x-retry-test-id", retryTest.id, "User-Agent", fmtUserAgent("test"));
-                }
-              })
+              FixedHeaderProvider.create(
+                  ImmutableMap.of(
+                      "x-retry-test-id", retryTest.id, "User-Agent", fmtUserAgent("test"))))
           .setRetrySettings(retrySettingsBuilder.setMaxAttempts(3).build());
     } else {
       builder
           .setHeaderProvider(
-              new FixedHeaderProvider() {
-                @Override
-                public Map<String, String> getHeaders() {
-                  return ImmutableMap.of("User-Agent", fmtUserAgent("non-test"));
-                }
-              })
+              FixedHeaderProvider.create(ImmutableMap.of("User-Agent", fmtUserAgent("non-test"))))
           .setRetrySettings(retrySettingsBuilder.setMaxAttempts(1).build());
     }
     return builder.build().getService();