Support for HTTP/3 (client side)

[reta]

Description

Support for HTTP/3 (client side) by introducing alternative, zero dependencies HTTP client built on top of JDK's HttpClient.

Motivation

Recently we have introduced an experimental HTTP/3 support on the served side. The client side turned out to be more complicated since the Apache HttpClient 5 that we rely on for rest client transport does not support HTTP/3 (and unluckily will in nearest future), see please https://issues.apache.org/jira/browse/HTTPCORE-793. This limitation leads to uneasy options:

• no support of the HTTP/3 on client side
• provide alternative client implementation to support HTTP/3 (and HTTP/1.1, HTTP/2, ... )

Alternative base clients

There are basically two production ready clients that fit into OpenSearch technical stack:

• Netty Client (and Reactor Netty that is built on top of it)
• OpenJDK HttpClient that supports HTTP/3 as of JDK-26 [1], [2]

OpenJDK looks particularly appealing since it requires no dependencies vs Netty that provides the foundation but requires more implementation work. Although our baseline is JDK-21, the applications could use JDK-26 (or later) and get HTTP/3 support for free, no changes required on OpenSearch side. Contrary, Netty's HTTP/3 implementation is built on top of native library (https://github.com/cloudflare/quiche) and is not available on many platforms.

Do we need new HTTP client?

The Rest Client is considered deprecated for a long time (we recommend to use opensearch-java), so we could consider it to be an internal change. Also, we stuck on Java 8 baseline for opensearch-java and HttpClient is only available in JDK-11, adding a support for it is possible as multi-release jar and will be added accordingly.

[1] #21170
[2] https://openjdk.org/jeps/517

Related Issues

Closes #20634

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit dcf871e.

Path	Line	Severity	Description
client/rest-http-client/build.gradle	38	high	New dependency added: io.projectreactor:reactor-core. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	39	high	New dependency added: org.reactivestreams:reactive-streams. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	33	high	New FIPS runtime dependency: org.bouncycastle:bc-fips. Cryptographic library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	34	high	New FIPS runtime dependency: org.bouncycastle:bctls-fips. TLS library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	35	high	New FIPS runtime dependency: org.bouncycastle:bcutil-fips. Cryptographic utility library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	30	high	New dependency added: commons-codec:commons-codec. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	31	high	New dependency added: commons-logging:commons-logging. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	32	high	New dependency added: org.slf4j:slf4j-api. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/licenses/reactor-core-3.8.5.jar.sha1	1	high	New SHA1 checksum file added for reactor-core-3.8.5. Maintainers must independently verify this hash matches the expected artifact from the official release.
client/rest-http-client/licenses/bc-fips-2.1.2.jar.sha1	1	high	New SHA1 checksum file added for bc-fips-2.1.2. Maintainers must independently verify this hash matches the expected BouncyCastle FIPS artifact.
settings.gradle	31	high	New Gradle subproject 'client:rest-http-client' registered. This activates all build.gradle dependency declarations in the new module, including all newly introduced third-party libraries.

The table above displays the top 10 most important findings.

Total: 11 | Critical: 0 | High: 11 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[reta]

@andrross @cwperks looking for early feedback folks. The pull request is not in ready state yet, mostly looking into "does it make sense or not" perspective, since we reach some limits of the client HTTP libraries we use. Thanks a lot, would really appreciate thoughts.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 08a54ad)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The getResult method at line 1220 always returns null. This method is passed as a finisher to BodyHandlers.fromSubscriber at line 969, meaning the HTTP response body will always be null regardless of what the subscriber receives. This breaks the async request handling for non-streaming requests. private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { return null; } Resource Leak In ContentCompressingEntity.getContentLength (lines 1142-1165), the method calls getContent() to read the entire stream for calculating length when chunked is disabled. However, if an IOException occurs during reading (line 1156), the method returns -1 but the InputStream from getContent() may not be properly closed, potentially leaking resources. The try-with-resources only closes the InputStream is, but if getContent() throws before returning, the underlying stream from the entity may remain open. public long getContentLength() { if (chunkedEnabled.isPresent()) { if (chunkedEnabled.get()) { return -1L; } else { long size = 0; final byte[] buf = new byte[8192]; int nread = 0; try (InputStream is = getContent()) { // read to EOF which may read more or less than buffer size while ((nread = is.read(buf)) > 0) { size += nread; } } catch (IOException ex) { size = -1L; } return size; } } else { return -1; } } Possible Issue In buildUri (lines 671-704), when constructing a new URI with query parameters, if the original URI already has a query string, the method appends additional parameters with "&". However, if additionalQuery is empty (no params in the map), this results in a trailing "&" or malformed query string like "existing&". This occurs when params is empty but uri.getQuery() is not null. String newQuery = uri.getQuery(); if (newQuery == null) { newQuery = additionalQuery; } else { newQuery += "&" + additionalQuery; } Possible Issue In WaitForCancelHandler.handle (lines 96-103), the method waits on cancelHandlerLatch but if interrupted, it swallows the InterruptedException and continues to send a 200 response. This can cause the test to pass incorrectly if the thread is interrupted, as the cancellation signal is lost and the handler behaves as if the request completed normally. public void handle(HttpExchange exchange) throws IOException { try { cancelHandlerLatch.await(); } catch (InterruptedException ignore) {} finally { exchange.sendResponseHeaders(200, 0); exchange.close(); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 08a54ad

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Stream consumed prematurely for length calculation Calling getContent() consumes the input stream, making it unavailable for the actual request. This will cause the request body to be empty when sent. Content length should be calculated before consuming the stream or cached during compression. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { if (chunkedEnabled.isPresent()) { if (chunkedEnabled.get()) { return -1L; } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; + // Calculate size during compression and cache it + // to avoid consuming the stream + return cachedContentLength; } } else { return -1; } } Suggestion importance[1-10]: 9 __ Why: Calling getContent() within getContentLength() consumes the input stream, making it unavailable for the actual request body. This is a critical bug that will cause empty request bodies to be sent.	High
	Method returns null unconditionally This method always returns null regardless of the subscriber parameter, which will likely cause NullPointerException when the result is used. Either implement proper logic to extract results from the subscriber or remove this unused method if it's not needed. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Remove this method if unused, or implement proper result extraction: private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { - return null; + throw new UnsupportedOperationException("Result extraction not implemented"); } Suggestion importance[1-10]: 7 __ Why: The method getResult always returns null, which could lead to NullPointerException when used. However, without understanding the full context of how this method is intended to be used or if it's actually called, the impact is uncertain.	Medium
	Handle null thread names The reject method doesn't handle null thread names, which could cause a NullPointerException if t.getName() returns null. Add a null check before comparing thread names to prevent potential crashes during thread leak detection. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [19-24] public boolean reject(Thread t) { String n = t.getName(); + if (n == null) { + return false; + } // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if t.getName() returns null. While Thread.getName() typically doesn't return null in practice, adding defensive null checking improves robustness in test infrastructure code.	Medium
	Prevent null pointer exception The reject method will throw a NullPointerException if t.getName() returns null. Add a null check before calling startsWith() to ensure robust thread filtering without runtime exceptions. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [20-23] public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException when calling startsWith() on a null thread name. Adding null checking prevents runtime exceptions and makes the thread filter more defensive.	Medium
General	Cache compressed content for reuse The entity input stream is consumed here and cannot be reused. If getContent() is called multiple times or after getContentLength(), the stream will be exhausted. Consider caching the compressed content or ensuring the stream is only consumed once. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1120-1126] +private volatile InputStream cachedContent; + public InputStream getContent() throws IOException { - ByteArrayInputOutputStream out = new ByteArrayInputOutputStream(1024); - try (GZIPOutputStream gzipOut = new GZIPOutputStream(out)) { - entity.transferTo(gzipOut); + if (cachedContent == null) { + synchronized (this) { + if (cachedContent == null) { + ByteArrayInputOutputStream out = new ByteArrayInputOutputStream(1024); + try (GZIPOutputStream gzipOut = new GZIPOutputStream(out)) { + entity.transferTo(gzipOut); + } + cachedContent = out.asInput(); + } + } } - return out.asInput(); + return cachedContent; } Suggestion importance[1-10]: 8 __ Why: The entity stream is consumed when getContent() is called, and subsequent calls or calls to getContentLength() will fail. Caching the compressed content prevents stream exhaustion issues and ensures the method can be called multiple times safely.	Medium
	Handle missing port in URI properly The mock HTTP client creates HttpHost with potentially -1 port from request.uri().getPort() when no port is specified. This may cause issues in tests expecting specific port values. Use a default port (e.g., 9200) when port is not present in the URI. client/rest-http-client/src/test/java/org/opensearch/httpclient/RestHttpClientSingleHostTests.java [216-218] private static HttpResponse<?> responseOrException(HttpRequest request) throws Exception { - final HttpHost httpHost = new HttpHost(request.uri().getScheme(), request.uri().getHost(), request.uri().getPort()); - // return the desired status code or exception depending on the path - switch (request.uri().getPath()) { - case "/soe": - throw new SocketTimeoutException(httpHost.toString()); - case "/coe": - throw new HttpTimeoutException(httpHost.toString()); + int port = request.uri().getPort(); + if (port == -1) { + port = "https".equals(request.uri().getScheme()) ? 443 : 9200; + } + final HttpHost httpHost = new HttpHost(request.uri().getScheme(), request.uri().getHost(), port); + ... Suggestion importance[1-10]: 5 __ Why: The suggestion to handle -1 port values is valid for robustness, but this is test code and the mock may intentionally use -1 to test edge cases. The impact is limited to test reliability.	Low

---

Previous suggestions

Suggestions up to commit 56f8647

Category	Suggestion	Impact
Possible issue	Stream consumed during length calculation The getContentLength() method consumes the entire input stream to calculate size, which means the stream cannot be read again for the actual request. This will cause the request body to be empty. Consider caching the compressed content or using a different approach to determine content length. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { - if (chunkedEnabled.isPresent()) { - if (chunkedEnabled.get()) { - return -1L; - } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; - } - } else { - return -1; + if (chunkedEnabled.isPresent() && chunkedEnabled.get()) { + return -1L; } + // Content length should be pre-calculated and cached + // Reading the stream here makes it unavailable for the actual request + return -1L; } Suggestion importance[1-10]: 9 __ Why: The getContentLength() method consumes the input stream to calculate size, making it unavailable for the actual request body. This is a critical issue that will cause empty request bodies when chunked encoding is disabled.	High
	Race condition in cancellation handling The streamRequest method sets a new cancellable future after the request is already in progress, which creates a race condition. If cancellation occurs between creating the future and calling setCancellable, the request cannot be cancelled. Set the cancellable before starting the async operation. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [335-346] private Publisher<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> streamRequest( final Node node, final Iterator<Node> nodes, final InternalStreamingRequest request ) throws IOException { return request.cancellable.callIfNotCancelled(() -> { final RequestContext<Flow.Publisher<List<ByteBuffer>>> context = request.createContextForNextAttempt(node); - final CompletableFuture<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> future = client.sendAsync( - context.requestProducer(), - context.asyncResponseConsumer() - ); + final CompletableFuture<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> future = new CompletableFuture<>(); request.setCancellable(future); + client.sendAsync(context.requestProducer(), context.asyncResponseConsumer()) + .whenComplete((result, error) -> { + if (error != null) future.completeExceptionally(error); + else future.complete(result); + }); Suggestion importance[1-10]: 8 __ Why: Setting the cancellable future after client.sendAsync creates a race condition where cancellation requests between these operations will fail. This affects the reliability of request cancellation.	Medium
	Method always returns null This method always returns null regardless of the subscriber parameter, which will likely cause NullPointerException when the result is used. Either implement proper logic to extract results from the subscriber or remove this unused method if it's not needed. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Remove this method if unused, or implement proper result extraction: private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { - return null; + throw new UnsupportedOperationException("Result extraction not implemented"); } Suggestion importance[1-10]: 7 __ Why: The getResult method at line 1220-1222 always returns null, which could cause NullPointerException when used. However, without seeing the full context of how this method is used, it's unclear if this is intentional or a bug.	Medium
	Handle null thread names The reject method doesn't handle null thread names, which could cause a NullPointerException if t.getName() returns null. Add a null check before comparing thread names to prevent potential runtime exceptions. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [19-24] public boolean reject(Thread t) { String n = t.getName(); + if (n == null) { + return false; + } // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if t.getName() returns null. While Thread.getName() typically doesn't return null in practice, adding defensive null checking improves code robustness in test utilities.	Medium
	Prevent null pointer exception The reject method will throw a NullPointerException if t.getName() returns null. Add a null check to safely handle threads with null names and prevent runtime exceptions. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [20-23] public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException risk when calling startsWith() on a potentially null thread name. Adding null safety improves the robustness of this thread filter implementation.	Medium
General	Resource leak in stream handling The entity InputStream is consumed during compression but never closed, which can lead to resource leaks. Ensure the input stream is properly closed after reading, especially if it represents a network connection or file handle. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1120-1126] public InputStream getContent() throws IOException { ByteArrayInputOutputStream out = new ByteArrayInputOutputStream(1024); - try (GZIPOutputStream gzipOut = new GZIPOutputStream(out)) { - entity.transferTo(gzipOut); + try (InputStream in = entity; GZIPOutputStream gzipOut = new GZIPOutputStream(out)) { + in.transferTo(gzipOut); } return out.asInput(); } Suggestion importance[1-10]: 7 __ Why: The entity InputStream is consumed but never explicitly closed, which could lead to resource leaks. While the suggestion is valid, the severity depends on whether the entity stream is managed elsewhere in the codebase.	Medium

Suggestions up to commit 25b49fb

Category	Suggestion	Impact
Possible issue	Prevent stream consumption in length calculation The getContentLength() method reads the entire compressed stream to calculate size when chunking is disabled. This consumes the stream, making it unavailable for the actual request. Consider caching the compressed content or calculating length before compression to avoid stream exhaustion. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { if (chunkedEnabled.isPresent()) { if (chunkedEnabled.get()) { return -1L; } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; + // Content length must be pre-calculated and cached + // to avoid consuming the stream + return cachedContentLength; } } else { return -1; } } Suggestion importance[1-10]: 9 __ Why: The getContentLength() method reads the entire stream to calculate size, which consumes the stream and makes it unavailable for the actual request. This is a critical bug that would prevent requests from working correctly when chunkedEnabled is set to false.	High
	Add null safety check Add null safety check for thread name before calling startsWith(). If t.getName() returns null, the current implementation will throw a NullPointerException. This could occur in edge cases with certain thread implementations. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [21-23] public final class HttpClientThreadLeakFilter implements ThreadFilter { @Override public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if t.getName() returns null. Adding a null check improves robustness, though this is an edge case in typical thread implementations.	Medium
General	Prevent memory exhaustion with large bodies The getContent() method in ContentCompressingEntity reads the entire input stream into memory before compression. For large request bodies, this can cause excessive memory consumption. Consider using streaming compression or implementing a size limit to prevent out-of-memory errors. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1120-1126] public InputStream getContent() throws IOException { + // Consider implementing streaming compression for large entities + // or add a size check to prevent memory exhaustion ByteArrayInputOutputStream out = new ByteArrayInputOutputStream(1024); try (GZIPOutputStream gzipOut = new GZIPOutputStream(out)) { entity.transferTo(gzipOut); } return out.asInput(); } Suggestion importance[1-10]: 7 __ Why: The getContent() method loads the entire input stream into memory before compression, which could cause memory issues with large request bodies. This is a valid concern for production systems handling large payloads, though it may not cause immediate failures in typical use cases.	Medium
	Ensure proper cancellation timing The streamRequest method sets the cancellable future after creating the context, but the future is created inside callIfNotCancelled. If cancellation occurs between these operations, the request may not be properly cancelled. Set the cancellable before starting the async operation to ensure proper cancellation handling. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [335-367] private Publisher<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> streamRequest( final Node node, final Iterator<Node> nodes, final InternalStreamingRequest request ) throws IOException { return request.cancellable.callIfNotCancelled(() -> { final RequestContext<Flow.Publisher<List<ByteBuffer>>> context = request.createContextForNextAttempt(node); final CompletableFuture<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> future = client.sendAsync( context.requestProducer(), context.asyncResponseConsumer() ); + // Set cancellable immediately after future creation request.setCancellable(future); ... }); } Suggestion importance[1-10]: 5 __ Why: The suggestion points out a potential race condition in cancellation handling. However, the setCancellable call is already immediately after the future creation within the callIfNotCancelled block, so the timing issue described may not be as severe as suggested. The improvement is valid but the impact is moderate.	Low
	Handle null thread name The current implementation doesn't handle the case where t.getName() returns null. While using equals() on string literals prevents NPE when n is null, it's safer to add an explicit null check for clarity and to prevent potential issues if the logic changes. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [20-23] @Override public boolean reject(Thread t) { String n = t.getName(); + if (n == null) { + return false; + } // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 5 __ Why: While the suggestion adds defensive programming, the current implementation using equals() on string literals already handles null safely. The explicit check adds clarity but provides minimal functional improvement.	Low
	Document or remove null-returning method This method always returns null and appears to be a placeholder or unused code. If this is intentional for compilation purposes, add a comment explaining why. Otherwise, remove it or implement the proper logic to avoid potential NullPointerExceptions when the return value is used. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Placeholder method required for type inference in BodyHandlers.fromSubscriber +// The actual result is obtained through AsyncResponseProducer.getResult() private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { return null; } Suggestion importance[1-10]: 3 __ Why: The method getResult always returns null and appears to be a placeholder. While adding documentation would improve code clarity, this is a minor improvement that doesn't address a critical issue or bug.	Low

Suggestions up to commit 20d8b95

Category	Suggestion	Impact
Possible issue	Stream consumed during length calculation The getContentLength() method consumes the entire input stream to calculate size, which means the stream cannot be read again for the actual request. This will cause the request body to be empty. Consider caching the compressed content or using a different approach to determine content length. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { - if (chunkedEnabled.isPresent()) { - if (chunkedEnabled.get()) { - return -1L; - } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; - } - } else { - return -1; + if (chunkedEnabled.isPresent() && chunkedEnabled.get()) { + return -1L; } + // Content length should be pre-calculated and cached + // to avoid consuming the stream + return -1L; // or throw exception if length is required } Suggestion importance[1-10]: 9 __ Why: The getContentLength() method in ContentCompressingEntity consumes the entire input stream to calculate size, making the stream unusable for the actual request body. This is a critical bug that will cause request bodies to be empty.	High
	Method always returns null This method always returns null regardless of the subscriber parameter, which will likely cause NullPointerExceptions when the result is used. Either implement proper logic to extract results from the subscriber or remove this unused method if it's not needed. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Remove this method if unused, or implement proper result extraction: private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { - return null; + throw new UnsupportedOperationException("Result extraction not implemented"); } Suggestion importance[1-10]: 7 __ Why: The getResult method always returns null, which will likely cause NullPointerException when used. This appears to be incomplete implementation that should either be properly implemented or removed if unused.	Medium
	Add null safety check Add null check for thread name before calling startsWith(). If t.getName() returns null, the method will throw a NullPointerException. This could happen in edge cases where thread names are not properly initialized. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [21-23] public final class HttpClientThreadLeakFilter implements ThreadFilter { @Override public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if t.getName() returns null. While rare, this is a valid defensive programming practice that improves robustness.	Medium
General	Unsafe URI resolution approach The URI resolution using URI.create(node.getHost().toString()).resolve(uri) may fail if the host string is not a valid URI or if the resolution doesn't work as expected. Use a more robust URI construction approach that properly combines scheme, host, port, and path components. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [664-669] private HttpRequest.Builder createHttpRequest(Node node, String method, URI uri, BodyPublisher body, Duration timeout) { + HttpHost host = node.getHost(); + String fullUri = host.scheme() + "://" + host.hostname() + ":" + host.port() + uri.toString(); return HttpRequest.newBuilder() - .uri(URI.create(node.getHost().toString()).resolve(uri)) + .uri(URI.create(fullUri)) .timeout(timeout) .method(method, (body == null) ? BodyPublishers.noBody() : body); } Suggestion importance[1-10]: 6 __ Why: The URI construction using URI.create(node.getHost().toString()).resolve(uri) may fail or produce incorrect results. A more explicit construction combining scheme, host, port, and path would be more robust and predictable.	Low
	Unsafe generic type casting The unchecked cast (HttpResponse) httpResponse is unsafe and may cause ClassCastException at runtime. The responseOrException method returns HttpResponse<?> but it's being cast to HttpResponse without verification that the body type matches the expected type T. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [194-210] -@SuppressWarnings("unchecked") -HttpClient mockHttpClient(final ExecutorService exec, final HttpClientListener... listeners) { - ... - @Override - public <T> CompletableFuture<HttpResponse<T>> sendAsync(HttpRequest request, BodyHandler<T> responseBodyHandler) { - Arrays.stream(listeners).forEach(l -> l.onExecute(request, responseBodyHandler)); +@Override +public <T> CompletableFuture<HttpResponse<T>> sendAsync(HttpRequest request, BodyHandler<T> responseBodyHandler) { + Arrays.stream(listeners).forEach(l -> l.onExecute(request, responseBodyHandler)); - final CompletableFuture<HttpResponse<T>> callback = new CompletableFuture<>(); - exec.submit(() -> { - try { - HttpResponse<?> httpResponse = responseOrException(request); - callback.complete((HttpResponse<T>) httpResponse); - return (T) httpResponse; - } catch (Exception e) { - callback.completeExceptionally(e); - return null; - } - }); + final CompletableFuture<HttpResponse<T>> callback = new CompletableFuture<>(); + exec.submit(() -> { + try { + HttpResponse<T> httpResponse = (HttpResponse<T>) responseOrException(request); + callback.complete(httpResponse); + return httpResponse.body(); + } catch (Exception e) { + callback.completeExceptionally(e); + return null; + } + }); - return callback; - } + return callback; } Suggestion importance[1-10]: 5 __ Why: The unchecked cast (HttpResponse<T>) httpResponse in the mock HTTP client is unsafe and suppresses warnings. While this is test code and may work in practice, it could cause ClassCastException if the body type doesn't match expectations.	Low
	Handle null thread name explicitly The current implementation doesn't handle the case where t.getName() returns null. While using equals() with a string literal on the left side prevents NPE, the variable n could still be null, making the logic unclear and potentially error-prone for future modifications. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [20-23] @Override public boolean reject(Thread t) { String n = t.getName(); + if (n == null) { + return false; + } // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 5 __ Why: While the suggestion adds explicit null handling, the current code already safely handles null through the string literal's equals() method. The improvement is marginal and mainly enhances code clarity.	Low

Suggestions up to commit 88c2605

Category	Suggestion	Impact
Possible issue	Prevent stream consumption in length calculation The getContentLength() method reads the entire stream to calculate size when chunking is disabled. This consumes the stream, making it unavailable for the actual request. This will cause the request body to be empty. Cache the content or calculate length before stream consumption. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { if (chunkedEnabled.isPresent()) { if (chunkedEnabled.get()) { return -1L; } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; + // Calculate length without consuming the stream + // or cache the content in a ByteArrayOutputStream + return -1L; // Return -1 to indicate unknown length } } else { return -1; } } Suggestion importance[1-10]: 9 __ Why: The getContentLength() method reads the entire input stream to calculate size when chunkedEnabled is false, which consumes the stream and makes it unavailable for the actual HTTP request body. This is a critical bug that would cause request bodies to be empty.	High
	Add null safety check Add null check for thread name before calling startsWith(). If t.getName() returns null, the method will throw a NullPointerException. This could happen in edge cases with certain thread implementations. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [21-23] public final class HttpClientThreadLeakFilter implements ThreadFilter { @Override public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for t.getName() prevents potential NullPointerException in edge cases. While Thread.getName() typically doesn't return null, defensive programming is valuable for test infrastructure code.	Medium
General	Ensure executor properly terminates in tests The executor service is shut down but not awaited for termination. This can lead to resource leaks or incomplete task execution in tests. Use awaitTermination() with a timeout to ensure clean shutdown before test completion. client/rest-http-client/src/test/java/org/opensearch/httpclient/RestHttpClientSingleHostTests.java [288-291] @After -public void shutdownExec() { +public void shutdownExec() throws InterruptedException { exec.shutdown(); + if (!exec.awaitTermination(10, TimeUnit.SECONDS)) { + exec.shutdownNow(); + } } Suggestion importance[1-10]: 7 __ Why: The test's @After method calls exec.shutdown() without waiting for termination, which could lead to resource leaks or incomplete task execution. Adding awaitTermination() ensures proper cleanup, though the impact is limited to test reliability rather than production code.	Medium
	Add null thread check The current implementation is safe against null thread names, but consider using a more defensive approach by checking if the thread parameter itself is null before accessing its name to prevent potential NullPointerException. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [20-24] @Override public boolean reject(Thread t) { + if (t == null) { + return false; + } String n = t.getName(); // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 6 __ Why: Adding a null check for the thread parameter t provides defensive programming. However, in the context of ThreadFilter implementations, the framework typically guarantees non-null threads, making this a minor improvement.	Low
	Plan migration from deprecated security API Using deprecated AccessController.doPrivileged with @SuppressWarnings("removal") indicates reliance on deprecated security APIs. Consider migrating to modern security mechanisms or document the migration plan, as this API is marked for removal in future Java versions. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [215-216] +// TODO: Migrate away from deprecated AccessController API +// Consider using SecurityManager alternatives or module-based security @SuppressWarnings("removal") HttpClient httpClient = AccessController.doPrivileged((PrivilegedAction<HttpClient>) this::createHttpClient); Suggestion importance[1-10]: 4 __ Why: The code uses deprecated AccessController.doPrivileged with suppression warnings. While this is a valid concern about future Java compatibility, it's already acknowledged with @SuppressWarnings("removal") and doesn't affect current functionality. The suggestion to add a TODO comment provides marginal value.	Low
	Remove or document unused method This method always returns null and appears to be a placeholder or unused code. If this is intentional for a callback mechanism, document its purpose. Otherwise, remove it to avoid confusion and potential null pointer issues if it's ever called. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Remove this method if unused, or document its purpose: +// This method is used as a completion handler for reactive streams private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { + // TODO: Implement proper result handling or remove if not needed return null; } Suggestion importance[1-10]: 3 __ Why: The method getResult at lines 1220-1222 always returns null and appears unused. While removing dead code improves maintainability, this has minimal impact since it's a private method that may be required for type inference in the BodyHandlers.fromSubscriber call at line 969.	Low

Suggestions up to commit a90bcb3

Category	Suggestion	Impact
Possible issue	Stream consumed during length calculation The getContentLength() method consumes the entire input stream to calculate size, which exhausts the stream and makes it unusable for the actual request. This will cause the request body to be empty. Content length should be calculated before stream consumption or cached, not by reading the stream each time. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1142-1165] public long getContentLength() { if (chunkedEnabled.isPresent()) { if (chunkedEnabled.get()) { return -1L; } else { - long size = 0; - final byte[] buf = new byte[8192]; - int nread = 0; - - try (InputStream is = getContent()) { - // read to EOF which may read more or less than buffer size - while ((nread = is.read(buf)) > 0) { - size += nread; - } - } catch (IOException ex) { - size = -1L; - } - - return size; + // Content length must be known beforehand or cached + // Reading the stream here makes it unusable for the actual request + throw new UnsupportedOperationException("Content length must be provided when chunked encoding is disabled"); } } else { return -1; } } Suggestion importance[1-10]: 9 __ Why: The getContentLength() method reads the entire input stream to calculate size, which exhausts the stream and makes it unusable for the actual HTTP request body. This is a critical bug that will cause request bodies to be empty.	High
	Method always returns null This method always returns null regardless of the subscriber parameter, which will likely cause NullPointerException when the result is used. The method signature suggests it should extract a result from the subscriber, but the implementation is incomplete. Either implement proper result extraction logic or remove this unused method. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [1220-1222] +// Remove this method if unused, or implement proper logic: private <T, S extends Subscriber<? super List<ByteBuffer>>> T getResult(S s1) { - return null; + throw new UnsupportedOperationException("Method not yet implemented"); } Suggestion importance[1-10]: 7 __ Why: The method getResult always returns null, which could cause NullPointerException when used. The implementation appears incomplete and should either be properly implemented or removed if unused.	Medium
	Add null safety check Add null safety check for thread name before calling startsWith(). If t.getName() returns null, the current implementation will throw a NullPointerException. This could occur in edge cases with certain thread implementations. client/rest-http-client/src/test/java/org/opensearch/httpclient/HttpClientThreadLeakFilter.java [21-23] public final class HttpClientThreadLeakFilter implements ThreadFilter { @Override public boolean reject(Thread t) { - return t.getName().startsWith("ForkJoinPool.commonPool-"); + String name = t.getName(); + return name != null && name.startsWith("ForkJoinPool.commonPool-"); } } Suggestion importance[1-10]: 7 __ Why: While Thread.getName() typically doesn't return null in standard JVM implementations, adding a null check prevents potential NullPointerException in edge cases and improves defensive programming practices.	Medium
General	URI resolution may produce incorrect paths The URI resolution using URI.create(node.getHost().toString()).resolve(uri) may produce incorrect results when the base URI doesn't end with a slash and the relative URI doesn't start with one. This can lead to path segments being incorrectly merged or replaced. Use proper URI construction to ensure correct path concatenation. client/rest-http-client/src/main/java/org/opensearch/httpclient/RestHttpClient.java [664-669] private HttpRequest.Builder createHttpRequest(Node node, String method, URI uri, BodyPublisher body, Duration timeout) { + String baseUri = node.getHost().toString(); + if (!baseUri.endsWith("/")) { + baseUri += "/"; + } + String path = uri.toString(); + if (path.startsWith("/")) { + path = path.substring(1); + } return HttpRequest.newBuilder() - .uri(URI.create(node.getHost().toString()).resolve(uri)) + .uri(URI.create(baseUri + path)) .timeout(timeout) .method(method, (body == null) ? BodyPublishers.noBody() : body); } Suggestion importance[1-10]: 6 __ Why: The URI resolution using URI.create().resolve() may produce incorrect results when path separators are not properly handled. However, the buildUri method at line 671 appears to handle path prefix concatenation separately, which may mitigate this issue.	Low
	Improve null handling clarity The current implementation is safe against null thread names due to using string literals on the left side of equals(). However, consider adding an explicit null check for consistency and clarity, especially since this is a cloned implementation that may be maintained separately. client/rest-http-client/src/test/java/org/opensearch/httpclient/BouncyCastleThreadFilter.java [20-23] @Override public boolean reject(Thread t) { String n = t.getName(); + if (n == null) { + return false; + } // Ignore BC's global background threads return "BC Disposal Daemon".equals(n) || "BC Cleanup Executor".equals(n); } Suggestion importance[1-10]: 3 __ Why: The current implementation using string literals on the left side of equals() already handles null safely. Adding an explicit null check provides marginal clarity improvement but doesn't fix an actual issue.	Low

[github-actions]

❌ Gradle check result for dcf871e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 5ea49ca

[github-actions]

❌ Gradle check result for 5ea49ca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 5ea49ca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 111049e

[github-actions]

❌ Gradle check result for 111049e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit a90bcb3

[github-actions]

Persistent review updated to latest commit 88c2605

[github-actions]

❌ Gradle check result for 88c2605: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 20d8b95

[github-actions]

❌ Gradle check result for 20d8b95: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 25b49fb

[github-actions]

❌ Gradle check result for 25b49fb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 56f8647

[github-actions]

❌ Gradle check result for 56f8647: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 08a54ad

[github-actions]

❌ Gradle check result for 08a54ad: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?