POC Option 1: Background REST Stub#13869
Conversation
…raries staging paths
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Resumable Upload Protocol (RUP) in the Java HTTP/JSON transport layer, including the core API abstractions in gax, the state machine implementation in gax-httpjson, and integration into the showcase stubs. The review feedback highlights several critical issues: a potential InputStream resource leak that needs a try-finally block, an insufficient handling of InterruptedException that could cause infinite retry loops, and a failure to propagate custom endpoints to the background HTTP/JSON stub. Additionally, improvements are suggested to prevent integer overflow during timeout configuration, trim cached memory buffers, and ensure exception-safe, LIFO-ordered resource shutdown.
| InputStream stream = uploadRequest.getStreamProvider().get(); | ||
| long streamPosition = 0; | ||
|
|
||
| List<BufferedChunk> cache = new ArrayList<>(); | ||
|
|
||
| // Phase 2 & 3 Loop: Transmit Chunks & Query Recovery | ||
| while (true) { | ||
| try { | ||
| checkDeadline(deadline); | ||
|
|
||
| // Find chunk in cache or read from stream | ||
| BufferedChunk chunk = null; | ||
| for (BufferedChunk cached : cache) { | ||
| if (cached.offset == offset) { | ||
| chunk = cached; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (chunk == null) { | ||
| // Read from stream | ||
| if (streamPosition != offset) { | ||
| if (stream != null) { | ||
| stream.close(); | ||
| } | ||
| stream = uploadRequest.getStreamProvider().get(); | ||
| long skipped = skipFully(stream, offset); | ||
| if (skipped < offset) { | ||
| throw new IOException("Failed to skip stream bytes to offset: " + offset); | ||
| } | ||
| streamPosition = offset; | ||
| } | ||
|
|
||
| byte[] buffer = new byte[adjustedChunkSize]; | ||
| int bytesRead = readFully(stream, buffer, adjustedChunkSize); | ||
| if (bytesRead > 0) { | ||
| chunk = new BufferedChunk(offset, buffer, bytesRead); | ||
| cache.add(chunk); | ||
| if (cache.size() > 2) { | ||
| cache.remove(0); | ||
| } | ||
| streamPosition += bytesRead; | ||
| } | ||
| } | ||
|
|
||
| if (chunk == null) { | ||
| // Stream was empty or exact chunk multiple and fully uploaded. | ||
| // Send finalize only | ||
| return sendFinalizeOnly(uploadUrl, offset, deadline); | ||
| } | ||
|
|
||
| // Check if this is the last chunk | ||
| boolean isEof = (chunk.length < adjustedChunkSize); | ||
|
|
||
| if (isEof) { | ||
| // Send upload, finalize for the last chunk | ||
| return sendUploadFinalize(uploadUrl, chunk.offset, chunk.data, chunk.length, deadline); | ||
| } | ||
|
|
||
| // Send intermediate chunk (upload command) | ||
| sendChunk(uploadUrl, chunk.offset, chunk.data, chunk.length, deadline); | ||
|
|
||
| // Successful chunk transmission! Update offset to next chunk | ||
| offset = chunk.offset + chunk.length; | ||
| attempt = 0; // Reset backoff attempts on progress | ||
|
|
||
| } catch (UploadAlreadyFinalizedException uafe) { | ||
| updateProgress( | ||
| uploadRequest.getTotalBytes() > 0 ? uploadRequest.getTotalBytes() : offset, | ||
| ResumableUploadProgressListener.State.COMPLETED); | ||
| return (ResponseT) uafe.getResponse(); | ||
| } catch (Exception e) { | ||
| checkDeadline(deadline); | ||
| ErrorCategory category = getErrorCategory(e); | ||
|
|
||
| if (category == ErrorCategory.CATEGORY_2_MISMATCH) { | ||
| logger.log(Level.WARNING, "State mismatch detected. Triggering recovery...", e); | ||
| updateProgress(offset, ResumableUploadProgressListener.State.RECOVERING); | ||
|
|
||
| long serverOffset = 0; | ||
| try { | ||
| serverOffset = recoverOffset(uploadUrl, deadline); | ||
| } catch (UploadAlreadyFinalizedException uafe) { | ||
| updateProgress( | ||
| uploadRequest.getTotalBytes() > 0 ? uploadRequest.getTotalBytes() : offset, | ||
| ResumableUploadProgressListener.State.COMPLETED); | ||
| return (ResponseT) uafe.getResponse(); | ||
| } | ||
|
|
||
| logger.log(Level.INFO, "Recovery completed. Server received bytes: {0}", serverOffset); | ||
|
|
||
| if (serverOffset == previousOffset) { | ||
| attempt++; | ||
| long delayMs = calculateBackoff(attempt); | ||
| sleep(delayMs); | ||
| } else { | ||
| attempt = 0; | ||
| previousOffset = serverOffset; | ||
| } | ||
|
|
||
| offset = serverOffset; | ||
| // Loop will handle finding the chunk in cache or seeking/recreating the stream! | ||
| } else if (category == ErrorCategory.CATEGORY_1_TRANSIENT) { | ||
| attempt++; | ||
| long delayMs = calculateBackoff(attempt); | ||
| logger.log( | ||
| Level.WARNING, | ||
| "Transient error. Backing off for {0} ms (attempt {1})", | ||
| new Object[] {delayMs, attempt}); | ||
| sleep(delayMs); | ||
| } else { | ||
| updateProgress(offset, ResumableUploadProgressListener.State.FAILED); | ||
| throw e; // Fatal, bubble up | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The InputStream opened at the start of runStateMachineInternal is never closed if the method exits normally or throws an exception. This will cause a resource leak. Wrap the entire loop in a try-finally block to ensure the stream is always closed properly.
InputStream stream = uploadRequest.getStreamProvider().get();
try {
long streamPosition = 0;
List<BufferedChunk> cache = new ArrayList<>();
// Phase 2 & 3 Loop: Transmit Chunks & Query Recovery
while (true) {
try {
checkDeadline(deadline);
// Find chunk in cache or read from stream
BufferedChunk chunk = null;
for (BufferedChunk cached : cache) {
if (cached.offset == offset) {
chunk = cached;
break;
}
}
if (chunk == null) {
// Read from stream
if (streamPosition != offset) {
if (stream != null) {
stream.close();
}
stream = uploadRequest.getStreamProvider().get();
long skipped = skipFully(stream, offset);
if (skipped < offset) {
throw new IOException("Failed to skip stream bytes to offset: " + offset);
}
streamPosition = offset;
}
byte[] buffer = new byte[adjustedChunkSize];
int bytesRead = readFully(stream, buffer, adjustedChunkSize);
if (bytesRead > 0) {
chunk = new BufferedChunk(offset, buffer, bytesRead);
cache.add(chunk);
if (cache.size() > 2) {
cache.remove(0);
}
streamPosition += bytesRead;
}
}
if (chunk == null) {
// Stream was empty or exact chunk multiple and fully uploaded.
// Send finalize only
return sendFinalizeOnly(uploadUrl, offset, deadline);
}
// Check if this is the last chunk
boolean isEof = (chunk.length < adjustedChunkSize);
if (isEof) {
// Send upload, finalize for the last chunk
return sendUploadFinalize(uploadUrl, chunk.offset, chunk.data, chunk.length, deadline);
}
// Send intermediate chunk (upload command)
sendChunk(uploadUrl, chunk.offset, chunk.data, chunk.length, deadline);
// Successful chunk transmission! Update offset to next chunk
offset = chunk.offset + chunk.length;
attempt = 0; // Reset backoff attempts on progress
} catch (UploadAlreadyFinalizedException uafe) {
updateProgress(
uploadRequest.getTotalBytes() > 0 ? uploadRequest.getTotalBytes() : offset,
ResumableUploadProgressListener.State.COMPLETED);
return (ResponseT) uafe.getResponse();
} catch (Exception e) {
checkDeadline(deadline);
ErrorCategory category = getErrorCategory(e);
if (category == ErrorCategory.CATEGORY_2_MISMATCH) {
logger.log(Level.WARNING, "State mismatch detected. Triggering recovery...", e);
updateProgress(offset, ResumableUploadProgressListener.State.RECOVERING);
long serverOffset = 0;
try {
serverOffset = recoverOffset(uploadUrl, deadline);
} catch (UploadAlreadyFinalizedException uafe) {
updateProgress(
uploadRequest.getTotalBytes() > 0 ? uploadRequest.getTotalBytes() : offset,
ResumableUploadProgressListener.State.COMPLETED);
return (ResponseT) uafe.getResponse();
}
logger.log(Level.INFO, "Recovery completed. Server received bytes: {0}", serverOffset);
if (serverOffset == previousOffset) {
attempt++;
long delayMs = calculateBackoff(attempt);
sleep(delayMs);
} else {
attempt = 0;
previousOffset = serverOffset;
}
offset = serverOffset;
// Loop will handle finding the chunk in cache or seeking/recreating the stream!
} else if (category == ErrorCategory.CATEGORY_1_TRANSIENT) {
attempt++;
long delayMs = calculateBackoff(attempt);
logger.log(
Level.WARNING,
"Transient error. Backing off for {0} ms (attempt {1})",
new Object[] {delayMs, attempt});
sleep(delayMs);
} else {
updateProgress(offset, ResumableUploadProgressListener.State.FAILED);
throw e; // Fatal, bubble up
}
}
}
} finally {
if (stream != null) {
try {
stream.close();
} catch (IOException e) {
logger.log(Level.WARNING, "Failed to close stream", e);
}
}
}| private void sleep(final long ms) { | ||
| try { | ||
| Thread.sleep(ms); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Catching InterruptedException and only restoring the interrupted flag inside sleep is insufficient because the outer retry loop does not check the thread's interrupted status. This can cause the thread to spin in an infinite retry loop when interrupted. Propagate InterruptedException so that the state machine terminates immediately upon interruption.
private void sleep(final long ms) throws InterruptedException {
Thread.sleep(ms);
}| if (clientContext.getCredentials() != null) { | ||
| TransportChannelProvider httpJsonProvider = | ||
| EchoStubSettings.defaultHttpJsonTransportProviderBuilder().build(); | ||
| ClientContext backgroundHttpContext = | ||
| clientContext.withTransportChannelProvider(httpJsonProvider); | ||
| this.httpJsonStub = new HttpJsonEchoStub(settings, backgroundHttpContext); | ||
| } else { | ||
| this.httpJsonStub = null; | ||
| } |
There was a problem hiding this comment.
The background HTTP/JSON stub is initialized using the default transport provider builder without applying any custom endpoint configured in settings. If a custom endpoint (such as a local emulator) is used, the background stub will incorrectly attempt to connect to the default production endpoint. Apply the custom endpoint to the provider builder.
if (clientContext.getCredentials() != null) {
com.google.api.gax.httpjson.InstantiatingHttpJsonChannelProvider.Builder httpJsonProviderBuilder =
EchoStubSettings.defaultHttpJsonTransportProviderBuilder();
if (settings.getEndpoint() != null) {
httpJsonProviderBuilder.setEndpoint(settings.getEndpoint());
}
TransportChannelProvider httpJsonProvider = httpJsonProviderBuilder.build();
ClientContext backgroundHttpContext =
clientContext.withTransportChannelProvider(httpJsonProvider);
this.httpJsonStub = new HttpJsonEchoStub(settings, backgroundHttpContext);
} else {
this.httpJsonStub = null;
}| byte[] buffer = new byte[adjustedChunkSize]; | ||
| int bytesRead = readFully(stream, buffer, adjustedChunkSize); | ||
| if (bytesRead > 0) { | ||
| chunk = new BufferedChunk(offset, buffer, bytesRead); | ||
| cache.add(chunk); | ||
| if (cache.size() > 2) { | ||
| cache.remove(0); | ||
| } | ||
| streamPosition += bytesRead; |
There was a problem hiding this comment.
If the last chunk read is smaller than adjustedChunkSize, the full buffer of size adjustedChunkSize is still retained in the cache. This can lead to high memory usage if adjustedChunkSize is large. Trim the buffer to the actual number of bytes read before caching.
| byte[] buffer = new byte[adjustedChunkSize]; | |
| int bytesRead = readFully(stream, buffer, adjustedChunkSize); | |
| if (bytesRead > 0) { | |
| chunk = new BufferedChunk(offset, buffer, bytesRead); | |
| cache.add(chunk); | |
| if (cache.size() > 2) { | |
| cache.remove(0); | |
| } | |
| streamPosition += bytesRead; | |
| byte[] buffer = new byte[adjustedChunkSize]; | |
| int bytesRead = readFully(stream, buffer, adjustedChunkSize); | |
| if (bytesRead > 0) { | |
| byte[] actualData = bytesRead < adjustedChunkSize | |
| ? java.util.Arrays.copyOf(buffer, bytesRead) | |
| : buffer; | |
| chunk = new BufferedChunk(offset, actualData, bytesRead); | |
| cache.add(chunk); | |
| if (cache.size() > 2) { | |
| cache.remove(0); | |
| } | |
| streamPosition += bytesRead; | |
| } |
| private void configureTimeouts(final HttpRequest request, final Instant deadline) { | ||
| long remainingMs = Duration.between(Instant.now(), deadline).toMillis(); | ||
| if (remainingMs <= 0) { | ||
| remainingMs = 1; // force timeout | ||
| } | ||
| request.setConnectTimeout((int) remainingMs); | ||
| request.setReadTimeout((int) remainingMs); | ||
| } |
There was a problem hiding this comment.
Casting remainingMs directly to int can cause an integer overflow if the remaining time is larger than Integer.MAX_VALUE (approx. 24.8 days). Cap the timeout value at Integer.MAX_VALUE to prevent negative or incorrect timeouts.
| private void configureTimeouts(final HttpRequest request, final Instant deadline) { | |
| long remainingMs = Duration.between(Instant.now(), deadline).toMillis(); | |
| if (remainingMs <= 0) { | |
| remainingMs = 1; // force timeout | |
| } | |
| request.setConnectTimeout((int) remainingMs); | |
| request.setReadTimeout((int) remainingMs); | |
| } | |
| private void configureTimeouts(final HttpRequest request, final Instant deadline) { | |
| long remainingMs = Duration.between(Instant.now(), deadline).toMillis(); | |
| if (remainingMs <= 0) { | |
| remainingMs = 1; // force timeout | |
| } | |
| int timeoutMs = (int) Math.min(remainingMs, Integer.MAX_VALUE); | |
| request.setConnectTimeout(timeoutMs); | |
| request.setReadTimeout(timeoutMs); | |
| } |
| @Override | ||
| public void shutdown() { | ||
| backgroundResources.shutdown(); | ||
| if (httpJsonStub != null) { | ||
| httpJsonStub.shutdown(); | ||
| } | ||
| } |
There was a problem hiding this comment.
When managing a collection of closeable resources, ensure they are shut down in the reverse order of their creation (LIFO) and in an exception-safe manner. Since httpJsonStub is initialized after backgroundResources, it should be shut down first. Wrap the shutdown logic in a try-finally block to ensure both resources are shut down.
@Override
public void shutdown() {
try {
if (httpJsonStub != null) {
httpJsonStub.shutdown();
}
} finally {
backgroundResources.shutdown();
}
}References
- When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.
Draft pull request showing the implementation of Option 1 (background REST stub delegation inside GrpcStub).