POC Option 3: Internal Standalone Upload Client#13870
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 gax-java by adding ResumableUploadCallable and its HTTP/JSON transport implementation. The review feedback identifies several critical issues and areas for improvement in HttpJsonResumableUploadCall and HttpJsonResumableUploadCallable. These include potential resource leaks from an unclosed InputStream, potential ClassCastException risks when handling headers and transport channels, inefficient JSON re-serialization and repeated HttpRequestFactory instantiation, a potential integer overflow when calculating timeouts for distant deadlines, and improper handling of InterruptedException in the sleep helper.
| attempt = 0; | ||
| long previousOffset = -1; | ||
|
|
||
| InputStream stream = uploadRequest.getStreamProvider().get(); |
There was a problem hiding this comment.
The InputStream obtained from uploadRequest.getStreamProvider().get() is opened before the main upload loop but is never closed when the method exits (either normally by returning a response or abruptly by throwing an exception). This can lead to resource leaks (e.g., open file descriptors or network connections). Wrap the stream usage in a try-finally block to ensure it is always closed properly.
| for (Map.Entry<String, Object> entry : requestHeaders.getHeaders().entrySet()) { | ||
| String key = entry.getKey(); | ||
| String value = (String) entry.getValue(); | ||
|
|
||
| if (isMetadataHeaderDenylisted(key)) { | ||
| httpRequest.getHeaders().set("X-Goog-Upload-Header-" + key, value); | ||
| } else { | ||
| httpRequest.getHeaders().set(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
The header values in requestHeaders.getHeaders() are stored as Object and can be of type List<String> (especially for multi-valued headers or extra headers from the call context). Casting them directly to (String) will throw a ClassCastException at runtime. Instead, handle both List and single String values safely.
for (Map.Entry<String, Object> entry : requestHeaders.getHeaders().entrySet()) {
String key = entry.getKey();
Object valueObj = entry.getValue();
if (valueObj instanceof List) {
for (Object val : (List<?>) valueObj) {
if (val != null) {
String value = val.toString();
if (isMetadataHeaderDenylisted(key)) {
httpRequest.getHeaders().set("X-Goog-Upload-Header-" + key, value);
} else {
httpRequest.getHeaders().set(key, value);
}
}
}
} else if (valueObj != null) {
String value = valueObj.toString();
if (isMetadataHeaderDenylisted(key)) {
httpRequest.getHeaders().set("X-Goog-Upload-Header-" + key, value);
} else {
httpRequest.getHeaders().set(key, value);
}
}
}|
|
||
| private void sleep(final long ms) { | ||
| try { | ||
| Thread.sleep(ms); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately, such as by throwing a relevant exception to signal that the operation cannot proceed.
References
- In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately, such as by throwing a relevant exception (e.g., SpannerException) to signal that the operation cannot proceed.
| HttpJsonTransportChannel transportChannel = | ||
| (HttpJsonTransportChannel) clientContext.getTransportChannel(); |
There was a problem hiding this comment.
If the clientContext passed to this callable is configured with a gRPC transport channel (which is the case when called from GrpcEchoStub if the background HTTP/JSON channel was not properly resolved), clientContext.getTransportChannel() will return a GrpcTransportChannel. Casting it directly to HttpJsonTransportChannel will throw a ClassCastException at runtime. Consider adding an explicit type check or ensuring that the background HTTP/JSON channel is fully resolved and wrapped before invoking the callable.
| if (!Strings.isNullOrEmpty(requestBody)) { | ||
| JSON_FACTORY.createJsonParser(requestBody).parse(tokenRequest); | ||
| initialContent = | ||
| new JsonHttpContent(JSON_FACTORY, tokenRequest) | ||
| .setMediaType(new HttpMediaType("application/json; charset=utf-8")); | ||
| } else { | ||
| initialContent = new EmptyContent(); | ||
| } |
There was a problem hiding this comment.
Parsing the already-serialized requestBody JSON string into a GenericData object only to re-serialize it using JsonHttpContent is highly inefficient. Since requestBody is already a valid JSON string, you can use ByteArrayContent.fromString directly to avoid this overhead.
if (!Strings.isNullOrEmpty(requestBody)) {
initialContent =
ByteArrayContent.fromString("application/json; charset=utf-8", requestBody);
} else {
initialContent = new EmptyContent();
}| private HttpRequestFactory getRequestFactory() { | ||
| Credentials credentials = callOptions.getCredentials(); | ||
| if (credentials != null) { | ||
| return httpTransport.createRequestFactory(new HttpCredentialsAdapter(credentials)); | ||
| } | ||
| return httpTransport.createRequestFactory(); | ||
| } |
There was a problem hiding this comment.
| 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.
If the configured deadline is very far in the future (e.g., more than 24 days), remainingMs can exceed Integer.MAX_VALUE. Casting it directly to (int) will cause an integer overflow, resulting in a negative timeout value which will throw an IllegalArgumentException or cause unexpected behavior. Clamp the value to Integer.MAX_VALUE to prevent this.
| 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 = (remainingMs > Integer.MAX_VALUE) ? Integer.MAX_VALUE : (int) remainingMs; | |
| request.setConnectTimeout(timeoutMs); | |
| request.setReadTimeout(timeoutMs); | |
| } |
Draft pull request showing the implementation of Option 3 (generic background HttpJsonResumableUploadClient delegation inside GrpcStub).