Skip to content

POC Option 1: Background REST Stub#13869

Draft
blakeli0 wants to merge 4 commits into
googleapis:mainfrom
blakeli0:feat/scotty-poc-option1
Draft

POC Option 1: Background REST Stub#13869
blakeli0 wants to merge 4 commits into
googleapis:mainfrom
blakeli0:feat/scotty-poc-option1

Conversation

@blakeli0

Copy link
Copy Markdown
Contributor

Draft pull request showing the implementation of Option 1 (background REST stub delegation inside GrpcStub).

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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.

Comment on lines +218 to +333
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
}
}
}

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.

high

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);
        }
      }
    }

Comment on lines +682 to +688
private void sleep(final long ms) {
try {
Thread.sleep(ms);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}

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.

high

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);
  }

Comment on lines +327 to +335
if (clientContext.getCredentials() != null) {
TransportChannelProvider httpJsonProvider =
EchoStubSettings.defaultHttpJsonTransportProviderBuilder().build();
ClientContext backgroundHttpContext =
clientContext.withTransportChannelProvider(httpJsonProvider);
this.httpJsonStub = new HttpJsonEchoStub(settings, backgroundHttpContext);
} else {
this.httpJsonStub = null;
}

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.

high

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;
    }

Comment on lines +251 to +259
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;

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.

medium

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.

Suggested change
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;
}

Comment on lines +623 to +630
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);
}

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.

medium

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.

Suggested change
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);
}

Comment on lines 673 to 679
@Override
public void shutdown() {
backgroundResources.shutdown();
if (httpJsonStub != null) {
httpJsonStub.shutdown();
}
}

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.

medium

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
  1. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant