feat(http): Add connection timeout - #339
Conversation
Bound connection establishment separately from the overall request timeout while preserving transport settings and honoring earlier request deadlines.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gavel summary
Totals: 499 passed · 0 failed · 3 skipped · 15.3s |
|
🎉 This PR is included in version 1.56.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@http/connect_test.go`:
- Around line 55-56: Reduce the upper-bound duration assertions around the
connection timing checks in the affected tests to a practical limit relative to
the configured 25-millisecond timeout, such as 250 milliseconds, while
preserving the existing lower-bound assertions and applying the same change to
both assertion pairs.
In `@http/connect.go`:
- Around line 27-35: Update ConnectTimeout’s transport-cloning logic to wrap
configured.DialTLSContext with the same timeout context used by DialContext,
preserving its existing fallback behavior as appropriate. Ensure HTTPS dialing
through a blocking custom DialTLSContext respects ConnectTimeout, and add a test
covering that timeout path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02a70f19-7702-45c5-a7e5-698ca03c927c
📒 Files selected for processing (2)
http/connect.gohttp/connect_test.go
| Expect(time.Since(started)).To(BeNumerically(">=", 25*time.Millisecond)) | ||
| Expect(time.Since(started)).To(BeNumerically("<", time.Second)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the timeout upper bounds.
Both assertions only require completion before one second. An implementation that delays dialing far beyond the configured 25 milliseconds can pass these tests.
Use a practical upper bound relative to the 25-millisecond timeout, such as 250 milliseconds.
Proposed test change
- Expect(time.Since(started)).To(BeNumerically("<", time.Second))
+ Expect(time.Since(started)).To(BeNumerically("<", 250*time.Millisecond))Also applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/connect_test.go` around lines 55 - 56, Reduce the upper-bound duration
assertions around the connection timing checks in the affected tests to a
practical limit relative to the configured 25-millisecond timeout, such as 250
milliseconds, while preserving the existing lower-bound assertions and applying
the same change to both assertion pairs.
| dialContext := configured.DialContext | ||
| if dialContext == nil { | ||
| dialContext = (&net.Dialer{}).DialContext | ||
| } | ||
| configured.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| connectCtx, cancel := context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
| return dialContext(connectCtx, network, address) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find existing custom TLS dialers that require timeout coverage.
rg -n -C 4 --type go '\bDialTLSContext\s*:|\bDialTLS\s*:' .Repository: flanksource/commons
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate connect.go =="
fd -a 'connect.go$' . | sed 's#^\./##'
echo "== http/connect.go snippet =="
if [ -f http/connect.go ]; then
nl -ba http/connect.go | sed -n '1,120p'
fi
echo "== search DialTLSContext usages =="
rg -n --type go 'DialTLSContext|DialTLS|DialContext|ConnectTimeout' . || true
echo "== git diff stat =="
git diff --stat || trueRepository: flanksource/commons
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate connect.go =="
fd -a 'connect.go$' . | sed 's#^\./##'
echo "== http/connect.go snippet =="
if [ -f http/connect.go ]; then
cat -n http/connect.go | sed -n '1,140p'
fi
echo "== search DialTLSContext usages =="
rg -n --type go 'DialTLSContext|DialTLS|DialContext|ConnectTimeout' . || true
echo "== lint availability =='
[ -f Makefile ] && rg -n 'lint|golangci' Makefile || trueRepository: flanksource/commons
Length of output: 2605
🌐 Web query:
Go net/http.Transport DialTLSContext used for HTTPS connections when non-nil documentation
💡 Result:
When net/http.Transport.DialTLSContext is set (non-nil), it is used to create TLS connections for non-proxied HTTPS requests [1][2]. Its behavior and interaction with other Transport settings are as follows: 1. Priority: If DialTLSContext is set, it takes precedence over the deprecated DialTLS hook [1][2]. 2. Bypassing Defaults: When DialTLSContext is provided, the Transport will not use the standard Dial or DialContext hooks for HTTPS requests [1][2]. Additionally, the TLSClientConfig and TLSHandshakeTimeout fields are ignored by the Transport, as the returned net.Conn is assumed to have already completed the TLS handshake [1][2]. 3. Expectation of State: The net.Conn returned by DialTLSContext is expected to be fully established and past the TLS handshake stage [1][2]. For proper ALPN (Application-Layer Protocol Negotiation) support, the returned connection should be a *tls.Conn or otherwise implement the ConnectionState method consistent with *tls.Conn [2]. 4. Fallback: If DialTLSContext is nil, the Transport checks for the deprecated DialTLS [1][2]. If both are nil, the Transport defaults to using DialContext (or Dial) combined with the TLSClientConfig to manage the connection and perform the handshake [1][2]. By using DialTLSContext, developers gain the ability to use the provided context.Context to cancel the dial operation when it is no longer needed, as well as the flexibility to perform custom TLS configuration or validation on a per-request basis [2][3].
Citations:
- 1: https://go.dev/src/net/http/transport.go?m=text
- 2: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 3: net/http: add DialTLSContext to Transport golang/go#21526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== http/connect_test.go =="
cat -n http/connect_test.go | sed -n '1,120p'
echo "== transport dial hook fields =="
go doc net/http.Transport DialTLSContext 2>/tmp/go_doc_err.log || cat /tmp/go_doc_err.logRepository: flanksource/commons
Length of output: 3395
Apply the timeout to DialTLSContext.
ConnectTimeout copies an *http.Transport but only wraps DialContext. When Transport.DialTLSContext is set, HTTPS connections use it instead of DialContext, so custom TLS dialing can bypass ConnectTimeout. Wrap the cloned DialTLSContext with the same timeout context and add an HTTPS test using a blocking DialTLSContext.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/connect.go` around lines 27 - 35, Update ConnectTimeout’s
transport-cloning logic to wrap configured.DialTLSContext with the same timeout
context used by DialContext, preserving its existing fallback behavior as
appropriate. Ensure HTTPS dialing through a blocking custom DialTLSContext
respects ConnectTimeout, and add a test covering that timeout path.
2 participants
What
Notes
Summary by CodeRabbit
New Features
Bug Fixes