[Java][apache-httpclient] Apply connect timeout to requests, add read timeout support#24284
Open
treaz wants to merge 3 commits into
Open
[Java][apache-httpclient] Apply connect timeout to requests, add read timeout support#24284treaz wants to merge 3 commits into
treaz wants to merge 3 commits into
Conversation
…ead timeout support setConnectTimeout() stored the value in a field that was never read, so the configured timeout was silently ignored and Apache HttpClient's own defaults applied (3-minute connect timeout, unbounded response wait). Wire connectionTimeout into a per-request RequestConfig set on the HttpClientContext in invokeAPI(), and add symmetric getReadTimeout()/ setReadTimeout() support mapped to the response timeout. The per-request approach works with both httpclient5 5.1.x (Gradle template) and 5.2.x (Maven template) and preserves a user-supplied custom CloseableHttpClient instead of rebuilding it. Fixes OpenAPITools#24269
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…faults, reject negative timeouts Address review feedback: - when building the per-request RequestConfig, start from the default request configuration of the underlying HTTP client (via Configurable) so a caller-supplied client's redirect policy, cookie spec and other request defaults are preserved and only the configured timeouts are overridden - reject negative values in setConnectTimeout()/setReadTimeout() with IllegalArgumentException instead of silently ignoring them
Author
|
Addressed the review findings in caedbe4:
|
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #24269.
For the Java client with
library=apache-httpclient,ApiClient.setConnectTimeout(int)stored the value in aconnectionTimeoutfield that was never read again, so the setter was a silent no-op: the client fell back to Apache HttpClient 5's own defaults (3-minute connect timeout, unbounded response wait).Changes
invokeAPI()now builds a per-requestRequestConfigfrom the configured timeouts and sets it on theHttpClientContext, sosetConnectTimeout(ms)actually bounds the TCP connect time, consistent with the other Java libraries (okhttp-gson, native).getReadTimeout()/setReadTimeout(ms)mapped to the response timeout, as suggested in the issue (mirrors the okhttp-gson naming).0(the default) leaves the underlying client's configuration untouched.The per-request
RequestConfigapproach was chosen over rebuilding the client with aPoolingHttpClientConnectionManager/ConnectionConfigbecause:ConnectionConfigonly exists since httpclient5 5.2, while the Gradle template still ships 5.1.3 (RequestConfig#setConnectTimeout(Timeout)exists since 5.0 and is honored in all 5.x versions, taking precedence over connection-manager defaults in 5.3+);CloseableHttpClient(constructor orsetHttpClient) instead of silently discarding it.Tests
Added
ApiClientTestto theapache-httpclientpetstore sample:All 136 test classes in the sample pass; the
echo_apiandjackson3samples were regenerated (./bin/generate-samples.sh bin/configs/java-apache-httpclient*.yaml) and compile.PR checklist
./bin/generate-samples.sh bin/configs/java-apache-httpclient*.yaml(only the apache-httpclient templates changed; no doc changes, soexport_docs_generators.shproduces no diff).cc @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @martin-mfg
Summary by cubic
Applies per-request connect and read timeouts in the Java
apache-httpclientclient to prevent hangs and makesetConnectTimeouteffective. Preserves defaults from a customCloseableHttpClient. Fixes #24269.Bug Fixes
setConnectTimeout(ms)per request to bound TCP connect time.RequestConfigfrom the underlying client's defaults (viaConfigurable); 0 uses client defaults.setConnectTimeout()/setReadTimeout().New Features
getReadTimeout()/setReadTimeout(ms)to control response wait time.Written for commit b3fa161. Summary will update on new commits.