Skip to content

Commit

Permalink
fix: update retry handling to retry idempotent requests that encounte…
Browse files Browse the repository at this point in the history
…r unexpected EOF while parsing json responses (#1155)

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

### Fix
Update DefaultStorageRetryStrategy to account for EOF errors from Json. When parsing json it's possible we've only received a partial document and parsing will fail. If an unexpected EOF happens from parsing and the request is idempotent retry it.

#### Tests
Add two new integration tests which leverage the testbench to force the EOF to happen upon the completion of the resumable session.
* Add 0B offset test
* Add 10B offset test

Add new cases to DefaultRetryHandlingBehaviorTest to ensure continued expected handling for the new EOF behavior.

#### Refactor
* Make TestBench.java (and its associated Builder) public to allow for use outside the retry conformance test package
* Create new JUnit @rule DataGeneration moving com.google.cloud.storage.it.ITStorageTest#randString to it and change the signature to produce a ByteBuffer rather than a string. (this should simplify use since the strings returned were immediately turned to bytes)

Fixes #1154

deps: update storage-testbench to v0.10.0

* chore: update testbench rule to use a different port than retry conformance tests
  • Loading branch information
BenWhitehead committed Dec 13, 2021
1 parent fe4b920 commit 8fbe6ef
Show file tree
Hide file tree
Showing 10 changed files with 471 additions and 86 deletions.
5 changes: 5 additions & 0 deletions google-cloud-storage/pom.xml
Expand Up @@ -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>
Expand Down
Expand Up @@ -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;

Expand All @@ -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);

Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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;
}
}
}
@@ -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();
}
};
}
}
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
}
Expand Up @@ -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
Expand All @@ -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();
}
};
}
}
Expand Up @@ -16,7 +16,7 @@

package com.google.cloud.storage.conformance.retry;

enum CleanupStrategy {
public enum CleanupStrategy {
ALWAYS,
ONLY_ON_SUCCESS,
NEVER
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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()
Expand All @@ -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();
Expand Down

0 comments on commit 8fbe6ef

Please sign in to comment.