Skip to content

Commit 72dc0df

Browse files
BenWhiteheadBenWhitehead
authored
fix: update request method of HttpStorageRpc to properly configure offset on requests (#1434)
* fix: update request method of HttpStorageRpc to properly configure offset on requests When invoking downloadTo(..., OutputStream) if a retry was attempted the proper byte offset was not being sent in the retried request. Update logic of HttpStorageRpc.read to manually set the range header rather than trying to rely on MediaDownloader to do it along with not automatically decompressing the byte stream. Update ITRetryConformanceTest to run Scenario 8 test cases, which cover resuming a download which could have caught this error sooner. Update StorageException.translate(IOException) to classify `IOException: Premature EOF` as the existing retryable reason `connectionClosedPrematurely`. Add case to DefaultRetryHandlingBehaviorTest to ensure conformance to this categorization. Break downloadTo integration test out into their own class, and separate the multiple scenarios being tested in the same method. Related to #1425
1 parent eac03a8 commit 72dc0df

File tree

8 files changed

+199
-72
lines changed

8 files changed

+199
-72
lines changed

‎google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,11 @@ static BaseServiceException coalesce(Throwable t) {
108108
* @returns {@code StorageException}
109109
*/
110110
public static StorageException translate(IOException exception) {
111-
if (exception.getMessage().contains("Connection closed prematurely")) {
112-
return new StorageException(
113-
0, exception.getMessage(), CONNECTION_CLOSED_PREMATURELY, exception);
111+
String message = exception.getMessage();
112+
if (message != null
113+
&& (message.contains("Connection closed prematurely")
114+
|| message.contains("Premature EOF"))) {
115+
return new StorageException(0, message, CONNECTION_CLOSED_PREMATURELY, exception);
114116
} else {
115117
// default
116118
return new StorageException(exception);

‎google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.api.client.googleapis.batch.BatchRequest;
2525
import com.google.api.client.googleapis.batch.json.JsonBatchCallback;
2626
import com.google.api.client.googleapis.json.GoogleJsonError;
27+
import com.google.api.client.googleapis.media.MediaHttpDownloader;
2728
import com.google.api.client.http.ByteArrayContent;
2829
import com.google.api.client.http.EmptyContent;
2930
import com.google.api.client.http.GenericUrl;
@@ -283,7 +284,7 @@ public void onFailure(GoogleJsonError googleJsonError, HttpHeaders httpHeaders)
283284
}
284285

285286
private static StorageException translate(IOException exception) {
286-
return new StorageException(exception);
287+
return StorageException.translate(exception);
287288
}
288289

289290
private static StorageException translate(GoogleJsonError exception) {
@@ -750,10 +751,14 @@ public long read(
750751
} else {
751752
req.setReturnRawInputStream(false);
752753
}
753-
req.getMediaHttpDownloader().setBytesDownloaded(position);
754-
req.getMediaHttpDownloader().setDirectDownloadEnabled(true);
754+
755+
if (position > 0) {
756+
req.getRequestHeaders().setRange(String.format("bytes=%d-", position));
757+
}
758+
MediaHttpDownloader mediaHttpDownloader = req.getMediaHttpDownloader();
759+
mediaHttpDownloader.setDirectDownloadEnabled(true);
755760
req.executeMedia().download(outputStream);
756-
return req.getMediaHttpDownloader().getNumBytesDownloaded();
761+
return mediaHttpDownloader.getNumBytesDownloaded();
757762
} catch (IOException ex) {
758763
span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage()));
759764
StorageException serviceException = translate(ex);

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

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.google.common.collect.ImmutableList.toImmutableList;
2020
import static com.google.common.collect.ImmutableSet.toImmutableSet;
2121
import static com.google.common.truth.Truth.assertWithMessage;
22+
import static org.junit.Assert.fail;
2223

2324
import com.fasterxml.jackson.core.JsonToken;
2425
import com.fasterxml.jackson.core.io.JsonEOFException;
@@ -96,6 +97,7 @@ public void validateBehavior() {
9697
} else if (shouldRetry && !defaultShouldRetryResult && !legacyShouldRetryResult) {
9798
actualBehavior = Behavior.SAME;
9899
message = "both are rejecting when we want a retry";
100+
fail(message);
99101
} else if (shouldRetry && defaultShouldRetryResult && legacyShouldRetryResult) {
100102
actualBehavior = Behavior.SAME;
101103
message = "both are allowing";
@@ -111,6 +113,7 @@ public void validateBehavior() {
111113
} else if (!shouldRetry && defaultShouldRetryResult && legacyShouldRetryResult) {
112114
actualBehavior = Behavior.SAME;
113115
message = "both are too permissive";
116+
fail(message);
114117
} else if (!shouldRetry && defaultShouldRetryResult && !legacyShouldRetryResult) {
115118
actualBehavior = Behavior.DEFAULT_MORE_PERMISSIBLE;
116119
message = "default is too permissive";
@@ -298,7 +301,7 @@ enum ThrowableCategory {
298301
STORAGE_EXCEPTION_GOOGLE_JSON_ERROR_503(new StorageException(C.JSON_503)),
299302
STORAGE_EXCEPTION_GOOGLE_JSON_ERROR_504(new StorageException(C.JSON_504)),
300303
STORAGE_EXCEPTION_SOCKET_TIMEOUT_EXCEPTION(new StorageException(C.SOCKET_TIMEOUT_EXCEPTION)),
301-
STORAGE_EXCEPTION_SOCKET_EXCEPTION(new StorageException(C.SOCKET_EXCEPTION)),
304+
STORAGE_EXCEPTION_SOCKET_EXCEPTION(StorageException.translate(C.SOCKET_EXCEPTION)),
302305
STORAGE_EXCEPTION_SSL_EXCEPTION(new StorageException(C.SSL_EXCEPTION)),
303306
STORAGE_EXCEPTION_SSL_EXCEPTION_CONNECTION_SHUTDOWN(
304307
new StorageException(C.SSL_EXCEPTION_CONNECTION_SHUTDOWN)),
@@ -322,6 +325,9 @@ enum ThrowableCategory {
322325
"connectionClosedPrematurely",
323326
"connectionClosedPrematurely",
324327
C.CONNECTION_CLOSED_PREMATURELY)),
328+
STORAGE_EXCEPTION_0_CONNECTION_CLOSED_PREMATURELY_IO_CAUSE_NO_REASON(
329+
StorageException.translate(C.CONNECTION_CLOSED_PREMATURELY)),
330+
STORAGE_EXCEPTION_0_IO_PREMATURE_EOF(StorageException.translate(C.IO_PREMATURE_EOF)),
325331
EMPTY_JSON_PARSE_ERROR(new IllegalArgumentException("no JSON input found")),
326332
JACKSON_EOF_EXCEPTION(C.JACKSON_EOF_EXCEPTION),
327333
STORAGE_EXCEPTION_0_JACKSON_EOF_EXCEPTION(
@@ -400,6 +406,7 @@ private static final class C {
400406
new JsonEOFException(null, JsonToken.VALUE_STRING, "parse-exception");
401407
private static final MalformedJsonException GSON_MALFORMED_EXCEPTION =
402408
new MalformedJsonException("parse-exception");
409+
private static final IOException IO_PREMATURE_EOF = new IOException("Premature EOF");
403410

404411
private static HttpResponseException newHttpResponseException(
405412
int httpStatusCode, String name) {
@@ -919,6 +926,28 @@ private static ImmutableList<Case> getAllCases() {
919926
HandlerCategory.NONIDEMPOTENT,
920927
ExpectRetry.NO,
921928
Behavior.DEFAULT_MORE_STRICT),
929+
new Case(
930+
ThrowableCategory
931+
.STORAGE_EXCEPTION_0_CONNECTION_CLOSED_PREMATURELY_IO_CAUSE_NO_REASON,
932+
HandlerCategory.IDEMPOTENT,
933+
ExpectRetry.YES,
934+
Behavior.SAME),
935+
new Case(
936+
ThrowableCategory
937+
.STORAGE_EXCEPTION_0_CONNECTION_CLOSED_PREMATURELY_IO_CAUSE_NO_REASON,
938+
HandlerCategory.NONIDEMPOTENT,
939+
ExpectRetry.NO,
940+
Behavior.DEFAULT_MORE_STRICT),
941+
new Case(
942+
ThrowableCategory.STORAGE_EXCEPTION_0_IO_PREMATURE_EOF,
943+
HandlerCategory.IDEMPOTENT,
944+
ExpectRetry.YES,
945+
Behavior.SAME),
946+
new Case(
947+
ThrowableCategory.STORAGE_EXCEPTION_0_IO_PREMATURE_EOF,
948+
HandlerCategory.NONIDEMPOTENT,
949+
ExpectRetry.NO,
950+
Behavior.DEFAULT_MORE_STRICT),
922951
new Case(
923952
ThrowableCategory.STORAGE_EXCEPTION_0_INTERNAL_ERROR,
924953
HandlerCategory.IDEMPOTENT,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ public static Collection<Object[]> testCases() throws IOException {
154154
.and(
155155
(m, trc) ->
156156
trc.getScenarioId()
157-
< 7) // Temporarily exclude resumable media scenarios
157+
!= 7) // Temporarily exclude resumable upload scenarios
158158
)
159159
.build();
160160

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

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.defaultSetup;
2020
import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.serviceAccount;
21+
import static com.google.common.base.Predicates.and;
2122
import static com.google.common.base.Predicates.not;
2223
import static com.google.common.truth.Truth.assertThat;
2324
import static org.junit.Assert.assertTrue;
@@ -66,6 +67,7 @@
6667
import com.google.cloud.storage.conformance.retry.RpcMethodMappings.Mappings.ObjectAcl;
6768
import com.google.cloud.storage.conformance.retry.RpcMethodMappings.Mappings.Objects;
6869
import com.google.cloud.storage.conformance.retry.RpcMethodMappings.Mappings.ServiceAccount;
70+
import com.google.common.base.Predicate;
6971
import com.google.common.collect.ImmutableList;
7072
import com.google.common.collect.ImmutableMap;
7173
import com.google.common.collect.ListMultimap;
@@ -108,6 +110,11 @@
108110
final class RpcMethodMappings {
109111
private static final Logger LOGGER = Logger.getLogger(RpcMethodMappings.class.getName());
110112

113+
private static final Predicate<TestRetryConformance> groupIsDownload =
114+
methodGroupIs("storage.objects.download");
115+
private static final Predicate<TestRetryConformance> groupIsResumableUpload =
116+
methodGroupIs("storage.resumable.upload");
117+
111118
static final int _2MiB = 2 * 1024 * 1024;
112119
final Multimap<RpcMethod, RpcMethodMapping> funcMap;
113120

@@ -1079,7 +1086,8 @@ private static void delete(ArrayList<RpcMethodMapping> a) {
10791086
private static void get(ArrayList<RpcMethodMapping> a) {
10801087
a.add(
10811088
RpcMethodMapping.newBuilder(39, objects.get)
1082-
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
1089+
.withApplicable(
1090+
and(not(TestRetryConformance::isPreconditionsProvided), not(groupIsDownload)))
10831091
.withSetup(defaultSetup.andThen(Local.blobInfoWithoutGeneration))
10841092
.withTest(
10851093
(ctx, c) ->
@@ -1088,13 +1096,15 @@ private static void get(ArrayList<RpcMethodMapping> a) {
10881096
.build());
10891097
a.add(
10901098
RpcMethodMapping.newBuilder(239, objects.get)
1091-
.withApplicable(TestRetryConformance::isPreconditionsProvided)
1099+
.withApplicable(
1100+
and(TestRetryConformance::isPreconditionsProvided, not(groupIsDownload)))
10921101
.withTest(
10931102
(ctx, c) ->
10941103
ctx.peek(state -> ctx.getStorage().get(state.getBlob().getBlobId())))
10951104
.build());
10961105
a.add(
10971106
RpcMethodMapping.newBuilder(40, objects.get)
1107+
.withApplicable(not(groupIsDownload))
10981108
.withTest(
10991109
(ctx, c) ->
11001110
ctx.map(
@@ -1108,6 +1118,7 @@ private static void get(ArrayList<RpcMethodMapping> a) {
11081118
.build());
11091119
a.add(
11101120
RpcMethodMapping.newBuilder(41, objects.get)
1121+
.withApplicable(not(groupIsDownload))
11111122
.withTest(
11121123
(ctx, c) ->
11131124
ctx.map(
@@ -1196,7 +1207,8 @@ private static void get(ArrayList<RpcMethodMapping> a) {
11961207
.build());
11971208
a.add(
11981209
RpcMethodMapping.newBuilder(60, objects.get)
1199-
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
1210+
.withApplicable(
1211+
and(not(TestRetryConformance::isPreconditionsProvided), not(groupIsDownload)))
12001212
.withTest((ctx, c) -> ctx.peek(state -> assertTrue(state.getBlob().exists())))
12011213
.build());
12021214
a.add(
@@ -1297,10 +1309,12 @@ private static void get(ArrayList<RpcMethodMapping> a) {
12971309
.build());
12981310
a.add(
12991311
RpcMethodMapping.newBuilder(75, objects.get)
1312+
.withApplicable(not(groupIsDownload))
13001313
.withTest((ctx, c) -> ctx.peek(state -> state.getBlob().reload()))
13011314
.build());
13021315
a.add(
13031316
RpcMethodMapping.newBuilder(76, objects.get)
1317+
.withApplicable(not(groupIsDownload))
13041318
.withTest(
13051319
(ctx, c) ->
13061320
ctx.peek(
@@ -1311,7 +1325,8 @@ private static void get(ArrayList<RpcMethodMapping> a) {
13111325
.build());
13121326
a.add(
13131327
RpcMethodMapping.newBuilder(107, objects.get)
1314-
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
1328+
.withApplicable(
1329+
and(not(TestRetryConformance::isPreconditionsProvided), not(groupIsDownload)))
13151330
.withTest(
13161331
(ctx, c) ->
13171332
ctx.map(state -> state.with(state.getBucket().get(c.getObjectName()))))
@@ -1321,7 +1336,8 @@ private static void get(ArrayList<RpcMethodMapping> a) {
13211336
private static void insert(ArrayList<RpcMethodMapping> a) {
13221337
a.add(
13231338
RpcMethodMapping.newBuilder(46, objects.insert)
1324-
.withApplicable(TestRetryConformance::isPreconditionsProvided)
1339+
.withApplicable(
1340+
and(TestRetryConformance::isPreconditionsProvided, not(groupIsResumableUpload)))
13251341
.withSetup(defaultSetup.andThen(Local.blobInfoWithGenerationZero))
13261342
.withTest(
13271343
(ctx, c) ->
@@ -1336,7 +1352,8 @@ private static void insert(ArrayList<RpcMethodMapping> a) {
13361352
.build());
13371353
a.add(
13381354
RpcMethodMapping.newBuilder(47, objects.insert)
1339-
.withApplicable(TestRetryConformance::isPreconditionsProvided)
1355+
.withApplicable(
1356+
and(TestRetryConformance::isPreconditionsProvided, not(groupIsResumableUpload)))
13401357
.withSetup(defaultSetup.andThen(Local.blobInfoWithGenerationZero))
13411358
.withTest(
13421359
(ctx, c) ->
@@ -1932,4 +1949,8 @@ private static void get(ArrayList<RpcMethodMapping> a) {
19321949
private static void put(ArrayList<RpcMethodMapping> a) {}
19331950
}
19341951
}
1952+
1953+
private static Predicate<TestRetryConformance> methodGroupIs(String s) {
1954+
return (c) -> s.equals(c.getMethod().getGroup());
1955+
}
19351956
}

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

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.cloud.conformance.storage.v1.InstructionList;
2525
import com.google.cloud.conformance.storage.v1.Method;
2626
import com.google.common.base.Joiner;
27+
import com.google.common.base.Suppliers;
2728
import com.google.errorprone.annotations.Immutable;
2829
import java.io.IOException;
2930
import java.io.InputStream;
@@ -37,7 +38,9 @@
3738
import java.time.ZoneId;
3839
import java.time.ZoneOffset;
3940
import java.time.format.DateTimeFormatter;
41+
import java.util.function.Supplier;
4042
import java.util.stream.Collectors;
43+
import java.util.stream.IntStream;
4144

4245
/**
4346
* An individual resolved test case correlating config from {@link
@@ -58,13 +61,16 @@ final class TestRetryConformance {
5861
BASE_ID = formatter.format(now).replaceAll("[:]", "").substring(0, 6);
5962
}
6063

64+
private static final int _512KiB = 512 * 1024;
65+
private static final int _8MiB = 8 * 1024 * 1024;
66+
6167
private final String projectId;
6268
private final String bucketName;
6369
private final String bucketName2;
6470
private final String userProject;
6571
private final String objectName;
6672

67-
private final byte[] helloWorldUtf8Bytes = "Hello, World!!!".getBytes(StandardCharsets.UTF_8);
73+
private final Supplier<byte[]> lazyHelloWorldUtf8Bytes;
6874
private final Path helloWorldFilePath = resolvePathForResource();
6975
private final ServiceAccountCredentials serviceAccountCredentials =
7076
resolveServiceAccountCredentials();
@@ -126,6 +132,33 @@ final class TestRetryConformance {
126132
String.format(
127133
"%s_s%03d-%s-m%03d_obj1",
128134
BASE_ID, scenarioId, instructionsString.toLowerCase(), mappingId);
135+
lazyHelloWorldUtf8Bytes =
136+
Suppliers.memoize(
137+
() -> {
138+
// define a lazy supplier for bytes.
139+
// Not all tests need data for an object, though some tests - resumable upload - needs
140+
// more than 8MiB.
141+
// We want to avoid allocating 8.1MiB for each test unnecessarily, especially since we
142+
// instantiate all permuted test cases. ~1000 * 8.1MiB ~~ > 8GiB.
143+
String helloWorld = "Hello, World!";
144+
int baseDataSize;
145+
switch (method.getName()) {
146+
case "storage.objects.insert":
147+
baseDataSize = _8MiB + 1;
148+
break;
149+
case "storage.objects.get":
150+
baseDataSize = _512KiB;
151+
break;
152+
default:
153+
baseDataSize = helloWorld.length();
154+
break;
155+
}
156+
int endInclusive = (baseDataSize / helloWorld.length());
157+
return IntStream.rangeClosed(1, endInclusive)
158+
.mapToObj(i -> helloWorld)
159+
.collect(Collectors.joining())
160+
.getBytes(StandardCharsets.UTF_8);
161+
});
129162
}
130163

131164
public String getProjectId() {
@@ -153,7 +186,7 @@ public String getObjectName() {
153186
}
154187

155188
public byte[] getHelloWorldUtf8Bytes() {
156-
return helloWorldUtf8Bytes;
189+
return lazyHelloWorldUtf8Bytes.get();
157190
}
158191

159192
public Path getHelloWorldFilePath() {

0 commit comments

Comments
 (0)