feat(httpclient): reuse httpclient #1409
Conversation
WalkthroughThe changes update AWS SDK dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service as service/http_client.go
participant Util as util/http_client.go
participant AWS as AWS Client
Caller->>Service: NewProxyHttpClient(proxyURL)
Service->>Util: GetHttpClient(proxyURL)
Util-->>Service: *http.Client or error
alt Success
Service-->>Caller: *http.Client
else Fallback
Service->>Service: Fallback to legacy logic
Service-->>Caller: *http.Client
end
Caller->>AWS: Initialize AWS client with custom *http.Client
Estimated code review effort3 (90–240 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
util/http_client.go (2)
17-21: Consider using more descriptive constant names.The constant names could be more explicit about their purpose in the HTTP client context.
-const ( - KeyOfMaxIdleConnsPerHost = "MAX_IDLE_CONNS_PER_HOST" - KeyOfMaxIdleConns = "MAX_IDLE_CONNS" - KeyOfIdleConnTimeout = "IDLE_CONN_TIMEOUT" -) +const ( + KeyOfMaxIdleConnsPerHost = "HTTP_MAX_IDLE_CONNS_PER_HOST" + KeyOfMaxIdleConns = "HTTP_MAX_IDLE_CONNS" + KeyOfIdleConnTimeout = "HTTP_IDLE_CONN_TIMEOUT" +)
41-43: Potential type assertion panic risk.The type assertion
client.(*http.Client)could panic if the stored value is not of the expected type, though this should be impossible given the current implementation.Consider using a type-safe approach:
- if client, ok := innerHttpClients.Load(proxyURL); ok { - return client.(*http.Client), nil - } + if client, ok := innerHttpClients.Load(proxyURL); ok { + if httpClient, ok := client.(*http.Client); ok { + return httpClient, nil + } + }service/http_client.go (1)
31-31: Consider using English comments for international codebases.The Chinese comments may not be accessible to all developers working on the codebase.
- // 2025-07-17 兼容模式，当获取不到 httpClient 时，使用默认业务逻辑返回的 httpClient + // Compatibility mode: fallback to original httpClient logic if new implementation fails- // 2025-07-17 兼容模式，当获取不到 httpClient 时，使用默认业务逻辑返回的 httpClient + // Compatibility mode: fallback to original httpClient logic if new implementation failsAlso applies to: 46-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(3 hunks)relay/channel/aws/relay-aws.go(2 hunks)service/http_client.go(2 hunks)util/http_client.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
relay/channel/aws/relay-aws.go (1)
service/http_client.go (1)
NewProxyHttpClient(44-99)
service/http_client.go (1)
util/http_client.go (1)
GetHttpClient(31-113)
🔇 Additional comments (11)
util/http_client.go (2)
23-29: LGTM! Good choice of data structures and defaults.The sync.Map is appropriate for concurrent access patterns and the default connection pool values are reasonable for most use cases.
64-71: Password extraction logic is correct.The authentication handling properly extracts username and password from the URL, with appropriate fallback for missing password.
relay/channel/aws/relay-aws.go (3)
7-8: LGTM! Good import organization.The import reordering improves readability by grouping standard library imports separately from third-party imports.
Also applies to: 13-13, 19-20
32-35: Excellent error handling for HTTP client creation.The early return on HTTP client creation failure prevents the AWS client from being created with invalid configuration, which is the correct approach.
40-40: Clean integration with new HTTP client infrastructure.The HTTP client is now properly configured with proxy support while maintaining the existing AWS SDK configuration pattern.
go.mod (3)
35-35: Good addition of golang.org/x/sync dependency.The explicit addition of golang.org/x/sync v0.11.0 as a direct dependency is appropriate since it's now used directly in the util package for sync.Map functionality.
44-53: New AWS SDK indirect dependencies are expected.The addition of new indirect dependencies like ec2/imds, internal modules, and service modules is normal when upgrading AWS SDK to newer versions that have better modularization.
10-12: AWS SDK v2 Core Upgrade Safe—but Verify Service-Specific ChangelogsNo breaking changes were introduced in the AWS SDK for Go v2 core modules between v1.26.1 and v1.36.5. All top-level updates in that range are feature additions, new service API support, and dependency bumps.
• No core-level breaking changes identified (v1.26.1 → v1.36.5)
• Ensure your project’s Go version meets any new minimums (e.g. some service clients now require Go 1.19+)
• Manually review the CHANGELOG.md for each AWS service you use—especially service/bedrockruntime—to catch any module-specific breaking changesservice/http_client.go (3)
39-41: Clean delegation to the new HTTP client utility.The helper function provides a clean abstraction layer for integrating with the new util.GetHttpClient functionality.
30-36: Good fallback strategy for backward compatibility.The fallback to the original httpClient when the new implementation fails ensures system stability during the transition period.
45-49: Consistent fallback pattern in NewProxyHttpClient.The same fallback approach is consistently applied in NewProxyHttpClient, maintaining reliability while enabling the new functionality.
| case "socks5", "socks5h": | ||
| // 获取认证信息 | ||
| var auth *proxy.Auth | ||
| if parsedURL.User != nil { | ||
| auth = &proxy.Auth{ | ||
| User: parsedURL.User.Username(), | ||
| Password: "", | ||
| } | ||
| if password, ok := parsedURL.User.Password(); ok { | ||
| auth.Password = password | ||
| } | ||
| } | ||
|
|
||
| // 创建 SOCKS5 代理拨号器 | ||
| // proxy.SOCKS5 使用 tcp 参数，所有 TCP 连接包括 DNS 查询都将通过代理进行。行为与 socks5h 相同 | ||
| dialer, err := proxy.SOCKS5("tcp", parsedURL.Host, auth, proxy.Direct) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| transport = &http.Transport{ | ||
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return dialer.Dial(network, addr) | ||
| }, | ||
| MaxIdleConns: maxIdleConns, | ||
| MaxIdleConnsPerHost: maxIdleConnsPerHost, | ||
| IdleConnTimeout: idleConnTimeout, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SOCKS5 proxy implementation behavior.
The comment on lines 74-75 states that proxy.SOCKS5 with "tcp" makes all connections including DNS queries go through the proxy (socks5h behavior), but this needs verification as the distinction between socks5 and socks5h schemes might not be properly handled.
🌐 Web query:
What is the difference between SOCKS5 and SOCKS5H proxy schemes and how does golang.org/x/net/proxy handle DNS resolution for each?
💡 Result:
The key difference between SOCKS5 and SOCKS5H proxy schemes centers on how DNS resolution is performed when making a proxied connection:
-
SOCKS5: The client (your local machine or application) performs DNS resolution. When you connect through a SOCKS5 proxy and provide a hostname, your client resolves that hostname to an IP address before sending the connection request to the proxy. The proxy then establishes a connection to the resolved IP. This method may expose your DNS queries locally, potentially leaking your intended targets[1][7].
-
SOCKS5H: The 'h' stands for hostname resolution by the proxy. When using SOCKS5H, your client sends the hostname (not the resolved IP address) to the proxy, which then performs the DNS lookup on your behalf and connects to the target host. This approach helps prevent DNS leaks and can be important for privacy and accessing geo-restricted content[1][7].
Here’s a table summarizing the difference:
| Scheme | DNS Resolution Location | Risk of Local DNS Leak | Use Case Example |
|---|---|---|---|
| SOCKS5 | Client-side | Yes | General proxying, but less privacy for DNS |
| SOCKS5H | Proxy-side | No | Privacy-sensitive tasks, geo-sensitive DNS |
How does golang.org/x/net/proxy handle each?
- The
golang.org/x/net/proxypackage only supports thesocks5://(SOCKS5) scheme and does not explicitly support thesocks5h://(SOCKS5H) scheme. When you provide a hostname, it is resolved locally before dialing through the proxy—consistent with standard SOCKS5 behavior[6].- For example, if you use the package’s
proxy.SOCKS5()function and pass a hostname, the Go DNS resolver is used, so DNS requests go out locally, not through the proxy[3][2]. - There is an open and longstanding issue where users have asked for support of
socks5h://so that hostname resolution can happen at the proxy. As of now, the package does not offer this functionality[6].
- For example, if you use the package’s
Summary:
- SOCKS5 resolves DNS locally (potential leaks).
- SOCKS5H resolves DNS at the proxy (no DNS leaks).
golang.org/x/net/proxydoes not currently support SOCKS5H: DNS will always be resolved locally by the client, regardless of the scheme[2][3][6][7].- To avoid DNS leaks in Go, you would need to implement or use a library that specifically supports proxy-side DNS resolution or use a workaround to ensure DNS queries are sent through the proxy.
Citations:
- 1: https://www.iproyal.net/en-us/blog/proxy-type-socks5
- 2: https://eli.thegreenplace.net/2022/go-and-proxy-servers-part-3-socks-proxies/
- 3: https://pkg.go.dev/golang.org/x/net/proxy
- 4: https://iproyal.com/blog/socks-vs-http-proxy/
- 5: https://pkg.go.dev/github.com/cloudfoundry/socks5-proxy
- 6: x/net/proxy: doesn't support socks5h:// protocol golang/go#13454
- 7: https://pingproxies.com/blog/socks-proxy-guide
- 8: https://github.com/armon/go-socks5
- 9: https://tip.golang.org/src/net/http/transport_test.go
- 10: https://surfshark.com/blog/socks-proxy
Correct SOCKS5 proxy DNS behavior comment
The current comment implying that proxy.SOCKS5 over “tcp” will proxy DNS lookups (i.e. socks5h behavior) is inaccurate. The golang.org/x/net/proxy package only implements standard SOCKS5, which always resolves hostnames locally before dialing. It does not support remote (proxy-side) DNS resolution (socks5h).
Please update the comment in util/http_client.go (around lines 74–75) accordingly. For example:
- // proxy.SOCKS5 使用 tcp 参数，所有 TCP 连接包括 DNS 查询都将通过代理进行。行为与 socks5h 相同
+ // proxy.SOCKS5 使用 tcp 参数代理 TCP 连接，但主机名解析仍在本地完成（无 socks5h 支持）If you need DNS resolution by the proxy, you’ll have to implement or use a library that explicitly supports SOCKS5H.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "socks5", "socks5h": | |
| // 获取认证信息 | |
| var auth *proxy.Auth | |
| if parsedURL.User != nil { | |
| auth = &proxy.Auth{ | |
| User: parsedURL.User.Username(), | |
| Password: "", | |
| } | |
| if password, ok := parsedURL.User.Password(); ok { | |
| auth.Password = password | |
| } | |
| } | |
| // 创建 SOCKS5 代理拨号器 | |
| // proxy.SOCKS5 使用 tcp 参数，所有 TCP 连接包括 DNS 查询都将通过代理进行。行为与 socks5h 相同 | |
| dialer, err := proxy.SOCKS5("tcp", parsedURL.Host, auth, proxy.Direct) | |
| if err != nil { | |
| return nil, err | |
| } | |
| transport = &http.Transport{ | |
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | |
| return dialer.Dial(network, addr) | |
| }, | |
| MaxIdleConns: maxIdleConns, | |
| MaxIdleConnsPerHost: maxIdleConnsPerHost, | |
| IdleConnTimeout: idleConnTimeout, | |
| } | |
| case "socks5", "socks5h": | |
| // 获取认证信息 | |
| var auth *proxy.Auth | |
| if parsedURL.User != nil { | |
| auth = &proxy.Auth{ | |
| User: parsedURL.User.Username(), | |
| Password: "", | |
| } | |
| if password, ok := parsedURL.User.Password(); ok { | |
| auth.Password = password | |
| } | |
| } | |
| // 创建 SOCKS5 代理拨号器 | |
| // proxy.SOCKS5 使用 tcp 参数代理 TCP 连接，但主机名解析仍在本地完成（无 socks5h 支持） | |
| dialer, err := proxy.SOCKS5("tcp", parsedURL.Host, auth, proxy.Direct) | |
| if err != nil { | |
| return nil, err | |
| } | |
| transport = &http.Transport{ | |
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | |
| return dialer.Dial(network, addr) | |
| }, | |
| MaxIdleConns: maxIdleConns, | |
| MaxIdleConnsPerHost: maxIdleConnsPerHost, | |
| IdleConnTimeout: idleConnTimeout, | |
| } |
🤖 Prompt for AI Agents
In util/http_client.go around lines 74 to 75, the comment incorrectly states
that proxy.SOCKS5 with "tcp" proxies DNS lookups like socks5h, but the
golang.org/x/net/proxy package only performs local DNS resolution before
dialing. Update the comment to clarify that DNS resolution is done locally and
that this implementation does not support remote DNS resolution (socks5h).
Mention that to achieve proxy-side DNS resolution, a different implementation or
library supporting SOCKS5H is required.
| innerHttpClients.Store(proxyURL, tmpClient) | ||
| return tmpClient, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memory management for the client cache.
The current implementation stores clients indefinitely in the sync.Map, which could lead to memory leaks if many different proxy URLs are used over time. Consider implementing a cleanup mechanism or LRU eviction.
Potential solutions:
- Add a periodic cleanup goroutine to remove unused clients
- Implement an LRU cache with size limits
- Add client reference counting with automatic cleanup
Do you want me to generate an implementation for any of these approaches?
🤖 Prompt for AI Agents
In util/http_client.go around lines 111 to 112, the current code stores HTTP
clients indefinitely in a sync.Map, risking memory leaks with many proxy URLs.
To fix this, implement a cache eviction strategy such as adding a periodic
cleanup goroutine to remove unused clients, or replace the sync.Map with an LRU
cache that limits size, or add reference counting to track client usage and
clean up when no longer needed. Choose one approach and modify the client
storage logic accordingly to prevent unbounded memory growth.
功能描述
1、复用http client
Summary by CodeRabbit
New Features
Bug Fixes
Chores