feat(ws): add WithClientProxy option for explicit proxy configuration#191
feat(ws): add WithClientProxy option for explicit proxy configuration#191qiushido wants to merge 1 commit intolarksuite:v3_mainfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds an explicit proxy configuration option for the WebSocket client by moving away from global singletons (ws.DefaultDialer, http.DefaultClient) and introducing per-client networking primitives.
Changes:
- Add per-
ws.Clientinstances of*websocket.Dialerand*http.Clientto avoid global-side effects. - Introduce
WithClientProxy(proxyUrl string)to apply an explicit proxy to both the WebSocket dialer and the HTTP client used to fetch the connection URL. - Route connection establishment and endpoint discovery through the client-owned dialer/http client.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| proxy, err := url.Parse(proxyUrl) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
url.Parse will succeed for many invalid proxy strings (e.g., missing scheme/host), and the resulting misconfiguration will only fail later at dial/request time. Consider validating that the parsed URL is absolute and has a supported scheme (http/https) and non-empty host, or using a stricter parser so invalid proxy inputs are rejected early.
| } | |
| } | |
| // Validate that the proxy URL is absolute, uses http/https, and has a host. | |
| if !proxy.IsAbs() || proxy.Host == "" { | |
| return | |
| } | |
| if proxy.Scheme != "http" && proxy.Scheme != "https" { | |
| return | |
| } |
| return | ||
| } | ||
| proxy, err := url.Parse(proxyUrl) | ||
| if err != nil { |
There was a problem hiding this comment.
When proxyUrl is non-empty but fails to parse, the option silently returns without applying any proxy. This makes misconfiguration hard to diagnose in production; consider surfacing this (e.g., store an error on the client to be returned from Start, or log a warning if a logger has already been provided).
| if err != nil { | |
| if err != nil { | |
| if cli.logger != nil { | |
| cli.logger.Warn(context.Background(), "invalid proxy URL %q: %v", proxyUrl, err) | |
| } |
| var transport *http.Transport | ||
| if t, ok := http.DefaultTransport.(*http.Transport); ok { | ||
| transport = t.Clone() | ||
| } else { | ||
| transport = &http.Transport{} | ||
| } |
There was a problem hiding this comment.
The fallback transport = &http.Transport{} does not inherit the sensible defaults used by http.DefaultTransport (notably Dial/TLS handshake timeouts). If http.DefaultTransport has been replaced with a non-*http.Transport, this fallback can lead to connections without timeouts and unexpected hangs; consider initializing the transport with the same defaults as the standard library’s default transport instead of a zero-value Transport.
| func WithClientProxy(proxyUrl string) ClientOption { | ||
| return func(cli *Client) { | ||
| if proxyUrl == "" { | ||
| return | ||
| } | ||
| proxy, err := url.Parse(proxyUrl) |
There was a problem hiding this comment.
WithClientProxy takes proxyUrl as a parameter name. For exported Go APIs, initialisms are typically capitalized (e.g., proxyURL) to match Go naming conventions and avoid inconsistent public surface naming.
| func WithClientProxy(proxyUrl string) ClientOption { | |
| return func(cli *Client) { | |
| if proxyUrl == "" { | |
| return | |
| } | |
| proxy, err := url.Parse(proxyUrl) | |
| func WithClientProxy(proxyURL string) ClientOption { | |
| return func(cli *Client) { | |
| if proxyURL == "" { | |
| return | |
| } | |
| proxy, err := url.Parse(proxyURL) |
当前版本的 websocket 客户端在建立长链接时,底层默认使用了 ws.DefaultDialer 和 http.DefaultClient。这导致了在某些需要通过代理访问企业内网或外部服务的场景下,开发者无法为 websocket 客户端单独、显式地配置网络代理。
💡 方案 (Solution)
在 ws.Client 结构体内维护私有的 dialer 和 httpClient 实例,取代原先的全局单例,避免并发情况下的全局污染。
新增 WithClientProxy(proxyUrl string) 配置项:
如果未显式调用 WithClientProxy,默认行为与之前保持一致(支持读取环境变量代理)。
💻 使用示例 (Example)