Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,16 @@
import com.google.api.core.ApiFutures;
import com.google.api.core.BetaApi;
import com.google.api.core.InternalApi;
import com.google.api.gax.resumable.ChunkUploadRequest;
import com.google.api.gax.resumable.ChunkUploadResponse;
import com.google.api.gax.resumable.ResumableUploadClient;
import com.google.api.gax.resumable.ResumableUploadSession;
import com.google.api.gax.retrying.ExponentialRetryAlgorithm;
import com.google.api.gax.retrying.RetryAlgorithm;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.retrying.ScheduledRetryingExecutor;
import java.io.InputStream;
import java.time.Duration;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;

Expand All @@ -54,9 +61,19 @@
public class ResumableUploadCallableImpl<RequestT, ResponseT>
extends ResumableUploadCallable<RequestT, ResponseT> {

private static final RetrySettings RETRY_SETTINGS =
RetrySettings.newBuilder()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least set initialRpcTimeoutDuration and totalTimeoutDuration. Otherwise the call could hang indefinitely (before global timeout kicks in). These are the default generated retrySetting values.

Separately, check if there is a cross-language sensible default.

.setInitialRetryDelayDuration(Duration.ofMillis(100))
.setRetryDelayMultiplier(1.3)
.setMaxRetryDelayDuration(Duration.ofMinutes(1))
.setMaxAttempts(5)
.build();

private final ResumableUploadClient<RequestT, ResponseT> client;
private final ResumableUploadCallSettings defaultCallSettings;
private final ClientContext clientContext;
private final UnaryCallable<ChunkUploadRequest, ChunkUploadResponse<ResponseT>>
retryingUploadChunkCallable;

public ResumableUploadCallableImpl(
ResumableUploadClient<RequestT, ResponseT> client,
Expand All @@ -66,6 +83,9 @@
this.defaultCallSettings =
checkNotNull(defaultCallSettings, "defaultCallSettings must not be null");
this.clientContext = checkNotNull(clientContext, "clientContext must not be null");
this.retryingUploadChunkCallable =
createRetryingCallable(
client.uploadChunkCallable(), ResumableUploadCommand.UPLOAD, clientContext);
}

@Override
Expand All @@ -88,7 +108,7 @@

return ResumableUploadFutureImpl.create(
startFuture,
client.uploadChunkCallable(),
retryingUploadChunkCallable,
payload,
effectiveSettings,
clientContext.getDefaultCallContext());
Expand All @@ -99,4 +119,18 @@
String sessionUrl, InputStream payload, @Nullable ResumableUploadCallSettings settings) {
throw new UnsupportedOperationException("Session resumption is not yet implemented.");
}

private static <ReqT, RespT> UnaryCallable<ReqT, RespT> createRetryingCallable(

Check warning on line 123 in sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ResumableUploadCallableImpl.java

View check run for this annotation

SonarQubeCloud / [gapic-generator-java-root] SonarCloud Code Analysis

Rename this generic name to match the regular expression '^[A-Z][0-9]?$'.

See more on https://sonarcloud.io/project/issues?id=googleapis_google-cloud-java_showcase&issues=AaDJ2VzPMdQCfkCO3QkV&open=AaDJ2VzPMdQCfkCO3QkV&pullRequest=14422

Check warning on line 123 in sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ResumableUploadCallableImpl.java

View check run for this annotation

SonarQubeCloud / [gapic-generator-java-root] SonarCloud Code Analysis

Rename this generic name to match the regular expression '^[A-Z][0-9]?$'.

See more on https://sonarcloud.io/project/issues?id=googleapis_google-cloud-java_showcase&issues=AaDJ2VzQMdQCfkCO3QkW&open=AaDJ2VzQMdQCfkCO3QkW&pullRequest=14422
UnaryCallable<ReqT, RespT> callable,
ResumableUploadCommand command,
ClientContext clientContext) {
RetryAlgorithm<RespT> retryAlgorithm =
new RetryAlgorithm<>(
new ResumableUploadResultRetryAlgorithm<>(command),
new ExponentialRetryAlgorithm(RETRY_SETTINGS, clientContext.getClock()));
return new RetryingCallable<>(
clientContext.getDefaultCallContext(),
checkNotNull(callable, "callable must not be null"),
new ScheduledRetryingExecutor<>(retryAlgorithm, clientContext.getExecutor()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.io.InputStream;
import java.util.Arrays;
import java.util.concurrent.CancellationException;
import java.util.concurrent.Executor;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;

Expand All @@ -59,6 +60,10 @@ final class ResumableUploadChunkCoordinator<ResponseT> {

private static final byte[] EMPTY_PAYLOAD = new byte[0];

// Serializes buffer mutations across multiple threads (i.e. from retry/recovery)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a case for "serializing buffer mutations across multiple threads"? retry/recovery is always sequential.

private final Executor chunkExecutor =
MoreExecutors.newSequentialExecutor(MoreExecutors.directExecutor());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to prevent stackoverflow of the sequential transmitChunk call? If it is, I don't think it would happen because each call will be run in a separate IOExecutor thread.


private final UnaryCallable<ChunkUploadRequest, ChunkUploadResponse<ResponseT>>
uploadChunkCallable;
private final String uploadUrl;
Expand Down Expand Up @@ -93,7 +98,7 @@ ApiFuture<ResponseT> start() {
}
},
MoreExecutors.directExecutor());
transmitChunk(0L);
chunkExecutor.execute(() -> transmitChunk(0L));
return result;
}

Expand Down Expand Up @@ -157,9 +162,10 @@ public void onSuccess(ChunkUploadResponse<ResponseT> response) {
result.setException(
new IllegalStateException(
"Upload stream ended and final chunk was transmitted, but server returned"
+ " incomplete status"));
+ " incomplete status for upload URL: "
+ uploadUrl));
Comment on lines 162 to +166

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Including the full uploadUrl in the exception message poses a security risk. Resumable upload URLs contain sensitive session IDs or tokens (such as upload_id in Google Cloud Storage) that act as bearer credentials. If this exception is logged or propagated to client applications, it could leak these credentials. Please revert to the original exception message or redact the sensitive query parameters from the URL before including it in the exception.

                result.setException(
                    new IllegalStateException(
                        "Upload stream ended and final chunk was transmitted, but server returned"
                            + " incomplete status"));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the URL in - it's a requirement that the URL is included in failure messages when possible. (I actually have a subsequent PR which expands that behavior).

} else {
transmitChunk(nextOffset);
chunkExecutor.execute(() -> transmitChunk(nextOffset));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@
void testResumableUploadCallable() {
ResumableUploadClient<String, String> uploadClient =
mock(ResumableUploadClient.class, Mockito.withSettings().withoutAnnotations());
when(uploadClient.uploadChunkCallable())
.thenReturn(mock(UnaryCallable.class, Mockito.withSettings().withoutAnnotations()));

Check warning on line 218 in sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/rpc/CallableTest.java

View check run for this annotation

SonarQubeCloud / [gapic-generator-java-root] SonarCloud Code Analysis

Extract this mock creation to a local variable.

See more on https://sonarcloud.io/project/issues?id=googleapis_google-cloud-java_showcase&issues=AaDGFYWiFZhPOSYKhVZo&open=AaDGFYWiFZhPOSYKhVZo&pullRequest=14422
ResumableUploadCallSettings settings =
ResumableUploadCallSettings.newBuilder().setChunkSize(1024).build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,102 @@ void testResumeCall_throwsUnsupportedOperationException() {
() -> callable.resumeCall("https://upload.url/session", streamOf("data"), null));
}

@Test
void testChunkRetry_transientFailureThenSuccess_retriesAndSucceeds() throws Exception {
stubStartSession("https://upload.url/chunk-retry-ok");
TrackableStream stream = new TrackableStream("01234567"); // exactly 1 chunk of 8 bytes
when(mockChunkCallable.futureCall(any(ChunkUploadRequest.class), any()))
.thenReturn(
ApiFutures.immediateFailedFuture(createApiException(503, StatusCode.Code.UNAVAILABLE)))
.thenReturn(
ApiFutures.immediateFuture(
ChunkUploadResponse.create(ResumableUploadStatus.FINAL, "chunk-done")));

ResumableUploadFuture<String> future = callable.futureCall("resource-path", stream, null);

assertThat(future.get()).isEqualTo("chunk-done");
assertThat(future.isDone()).isTrue();
assertThat(stream.totalBytesRead).isEqualTo(8);
assertThat(stream.closed).isTrue();

ArgumentCaptor<ChunkUploadRequest> captor = ArgumentCaptor.forClass(ChunkUploadRequest.class);
verify(mockChunkCallable, times(2)).futureCall(captor.capture(), any());
List<ChunkUploadRequest> requests = captor.getAllValues();
assertThat(requests.get(0).getOffset()).isEqualTo(0);
assertThat(requests.get(0).getPayload()).isEqualTo("01234567".getBytes(StandardCharsets.UTF_8));
assertThat(requests.get(1).getOffset()).isEqualTo(0);
assertThat(requests.get(1).getPayload()).isEqualTo("01234567".getBytes(StandardCharsets.UTF_8));
}

@Test
void testChunkRetry_transientFailureExhaustion_surfacesLastError() {
stubStartSession("https://upload.url/chunk-exhaustion");
when(mockChunkCallable.futureCall(any(ChunkUploadRequest.class), any()))
.thenReturn(
ApiFutures.immediateFailedFuture(createApiException(503, StatusCode.Code.UNAVAILABLE)));

ResumableUploadFuture<String> future =
callable.futureCall("resource-path", streamOf("hello"), null);

ExecutionException exception = assertThrows(ExecutionException.class, future::get);
assertThat(exception.getCause()).isInstanceOf(ApiException.class);
assertThat(((ApiException) exception.getCause()).getStatusCode().getTransportCode())
.isEqualTo(503);

// Default chunk retry settings has maxAttempts = 5
verify(mockChunkCallable, times(5)).futureCall(any(), any());
}

@Test
void testChunkRetry_cancellationDuringBackoff_deschedulesPendingAttempt() {
stubStartSession("https://upload.url/cancel-backoff");
SettableApiFuture<ChunkUploadResponse<String>> chunkAttempt0Future = SettableApiFuture.create();
when(mockChunkCallable.futureCall(any(ChunkUploadRequest.class), any()))
.thenReturn(chunkAttempt0Future)
.thenReturn(
ApiFutures.immediateFuture(
ChunkUploadResponse.create(ResumableUploadStatus.FINAL, "should-not-reach")));

ResumableUploadFuture<String> sessionFuture =
callable.futureCall("resource-path", streamOf("hello"), null);

// Fail attempt 0 with 503 to schedule backoff
chunkAttempt0Future.setException(createApiException(503, StatusCode.Code.UNAVAILABLE));

// Cancel while backoff is pending
assertThat(sessionFuture.cancel(true)).isTrue();
assertThat(sessionFuture.isCancelled()).isTrue();
assertThrows(CancellationException.class, sessionFuture::get);

// Only attempt 0 occurred; attempt 1 was de-scheduled
verify(mockChunkCallable, times(1)).futureCall(any(), any());
}

private static class HttpStatusStatusCode implements StatusCode {
private final int httpStatus;
private final StatusCode.Code code;

HttpStatusStatusCode(int httpStatus, StatusCode.Code code) {
this.httpStatus = httpStatus;
this.code = code;
}

@Override
public StatusCode.Code getCode() {
return code;
}

@Override
public Integer getTransportCode() {
return httpStatus;
}
}

private static ApiException createApiException(int httpStatus, StatusCode.Code code) {
return ApiExceptionFactory.createException(
"HTTP " + httpStatus, null, new HttpStatusStatusCode(httpStatus, code), false);
}

private void stubStartSession(String uploadUrl) {
when(mockStartCallable.futureCall(any(), any()))
.thenReturn(
Expand All @@ -424,11 +520,21 @@ private static void assertChunk(

private static class TrackableStream extends ByteArrayInputStream {
boolean closed = false;
int totalBytesRead = 0;

TrackableStream(String content) {
super(content.getBytes(StandardCharsets.UTF_8));
}

@Override
public int read(byte[] b, int off, int len) {
int read = super.read(b, off, len);
if (read > 0) {
totalBytesRead += read;
}
return read;
}

@Override
public void close() throws IOException {
closed = true;
Expand Down
Loading