chore(deps): Bump org.apache.httpcomponents.client5:httpclient5 - #1239
Conversation
Bumps [org.apache.httpcomponents.client5:httpclient5](https://github.com/apache/httpcomponents-client) from 5.3.1 to 5.6.3. - [Changelog](https://github.com/apache/httpcomponents-client/blob/rel/v5.6.3/RELEASE_NOTES.txt) - [Commits](apache/httpcomponents-client@rel/v5.3.1...rel/v5.6.3) --- updated-dependencies: - dependency-name: org.apache.httpcomponents.client5:httpclient5 dependency-version: 5.6.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request updates the httpclient5 dependency from version 5.3.1 to 5.6.3 in pom.xml. Consequently, the test server initialization in ApacheHttp2TransportTest.java has been refactored to use ServerBootstrap instead of the direct HttpServer constructor. The feedback suggests simplifying the anonymous HttpProcessor implementation in the test setup by using HttpProcessorBuilder to reduce boilerplate code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request upgrades the httpclient5 dependency to version 5.6.3 and implements read timeout handling in ApacheHttp2Request by tracking the timeout value and applying it to the responseFuture.get() call. It also adds a corresponding unit test to verify the timeout behavior and refactors the FakeServer test helper to use ServerBootstrap. There are no review comments, and I have no feedback to provide.
jonathanedey
left a comment
There was a problem hiding this comment.
LGTM with one nit, Thanks!
| throw new IOException("Request Cancelled", e); | ||
| } catch (TimeoutException e) { | ||
| responseFuture.cancel(true); | ||
| throw new IOException("Stream exception in request", e); |
There was a problem hiding this comment.
Should we specify that it was a read timeout here?
There was a problem hiding this comment.
Yeah good call! I thought about it but I wonder if that would break the backward compatibility in some way... even if it did, it would most likely be a minor risk I think
There was a problem hiding this comment.
Let's leave it like this for now because our priority is to get the dependency bumped. We can address that in a future PR.
Fixes the CI build failure in #1238 when bumping
org.apache.httpcomponents.client5:httpclient5from5.3.1to5.6.3.Enforce Read Timeout in
ApacheHttp2Request:InternalHttpAsyncExecRuntimeno longer sets socket-level timeouts on the endpoint when the protocol is HTTP/2 (version >= HTTP_2) because multiple multiplexed streams share a single connection. Consequently, HTTP/2 requests inApacheHttp2Requestwere waiting indefinitely onresponseFuture.get(), causing read timeout tests (ApacheHttp2TransportIT.testReadTimeoutAuthorizedGetandtestReadTimeoutAuthorizedPost) to fail.readTimeoutinApacheHttp2Requestand applied it toresponseFuture.get(readTimeout, TimeUnit.MILLISECONDS). On timeout, the future is cancelled (responseFuture.cancel(true)sends an HTTP/2 stream reset) and anIOException("Stream exception in request")is thrown to maintain backward compatibility with SDK error contracts. Added a unit testtestReadTimeoutinApacheHttp2TransportTest.Update Test
HttpServerInitialization inApacheHttp2TransportTest:HttpConnectionFactory) to the low-levelHttpServerconstructor, causing a compile failure inFakeServer.FakeServerto construct the server usingServerBootstrap.bootstrap().