Repository navigation
fix(http): honor HttpClient connect_timeout above 10s - #906
Merged
Merged
Conversation
http_client_connect started blocktime at DEFAULT_CONNECT_TIMEOUT (10s) and only ever lowered it with MIN(), so a connect_timeout above 10s was silently capped. The same budget also covers the HTTP CONNECT tunnel and the TLS handshake. Use connect_timeout as-is, falling back to the default only when it is unset, and cap it by the total timeout only when one is set. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The timeout calculation now matches the documented behavior without introducing unresolved issues.
0 open findings
What changed in this PR
Fixes synchronous HTTP connection timeouts above 10 seconds while preserving total-timeout limits.
Changes:
- Uses the configured connection timeout directly.
- Falls back to 10 seconds only when unset.
- Caps connection time by the total timeout when enabled.
| File | Description |
|---|---|
http/client/HttpClient.cpp |
Corrects synchronous connection timeout calculation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
http_client_connectcapped the connect timeout at 10s:blocktimestarted atDEFAULT_CONNECT_TIMEOUTand was only ever lowered withMIN(), soreq->connect_timeout(or thetimeoutargument ofHttpClient::connect) above 10s had no effect. The same budget also covers the HTTP CONNECT tunnel and the TLS handshake.connect_timeoutas-is, falling back to the default only when it is unset, and cap it by the totaltimeoutonly when one is set. This also makesconnect_timeouttake effect when the total timeout is disabled on both the request and the client, which previously fell back to 10s.AsyncHttpClientalready usesconnect_timeoutdirectly and is unchanged.Fixes #894
Verification
Connecting to
192.0.2.1:80(RFC 5737 documentation range, unroutable) so the connect hangs until the timeout fires, on macOS:Plain HTTP and HTTPS requests with the sync client, and HTTPS with the async client, still succeed against a local origin.
Note
The async client's total wait for an unreachable host includes its default retry (
retry_count = 1,retry_delay = 1000ms), e.g.connect_timeout = 13gives up after ~27s, while the sync client does not retry a failed connect. That difference is unchanged here.