added Close to fix quic goroutine leak#228
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request aims to fix a goroutine leak in QUIC/HTTP3 connections by adding a Close() method to the HttpClient interface and passing transport references through the protocol racing mechanism. However, the PR contains several critical issues including incomplete goroutine leak fixes, removal of established features without proper cleanup, and code that will not compile.
Changes:
- Added Close() method to HttpClient interface to properly close HTTP/3 transports
- Modified protocol racing to pass transport instances through racingResult for caching
- Removed pre-request and post-response hooks implementation and WithDialContext support
- Replaced automatic charset detection with hardcoded EUC-KR encoding (incomplete implementation)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| racer.go | Modified racingResult struct to include transport field and updated cacheWinningProtocol to reuse transport from race instead of creating new one |
| client.go | Added Close() method to interface and implementation, removed hooks execution logic, removed WithDialContext implementation, replaced charset detection with EUC-KR encoding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -164,7 +164,7 @@ func (pr *protocolRacer) attemptHTTP3(req *http.Request, resultCh chan<- racingR | |||
| if err != nil { | |||
| resultCh <- racingResult{protocol: "h3", err: fmt.Errorf("HTTP/3 request failed: %w", err)} | |||
There was a problem hiding this comment.
When the HTTP/3 request fails, the transport is not being passed in the result, which means the created HTTP/3 transport cannot be properly closed and will leak goroutines. The transport should be included in the error case to allow cleanup in waitForRaceWinner. Consider including the transport even on error so it can be properly closed.
| resultCh <- racingResult{protocol: "h3", err: fmt.Errorf("HTTP/3 request failed: %w", err)} | |
| resultCh <- racingResult{protocol: "h3", err: fmt.Errorf("HTTP/3 request failed: %w", err), transport: h3Transport} |
| @@ -160,25 +149,9 @@ func validateConfig(config *httpClientConfig) error { | |||
| return fmt.Errorf("invalid config: cannot set both proxy URL and custom proxy dialer factory (only one will be used)") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The removal of dialContext validation logic could allow invalid configurations to pass validation. The check that prevented combining WithDialContext with proxy settings has been removed, which was previously documented as important because custom dialers override proxy logic. This could lead to silent failures or unexpected behavior for users trying to use proxies with custom dialers.
| if config.dialer != nil && (config.proxyUrl != "" || config.proxyDialerFactory != nil) { | |
| return fmt.Errorf("invalid config: cannot use a custom dialer together with proxy settings (proxy would be overridden by the dialer)") | |
| } |
| @@ -614,6 +469,14 @@ func (c *httpClient) do(req *http.Request) (*http.Response, error) { | |||
|
|
|||
| finalResponse := string(buf) | |||
|
|
|||
| if c.config.euckrResponse { | |||
| var bufs bytes.Buffer | |||
| wr := transform.NewWriter(&bufs, korean.EUCKR.NewDecoder()) | |||
| wr.Write(buf) | |||
| wr.Close() | |||
| finalResponse = bufs.String() | |||
| } | |||
There was a problem hiding this comment.
The removal of automatic charset detection (using charset.NewReader) and its replacement with hardcoded korean.EUCKR encoding is problematic. The original code automatically detected and handled various character encodings based on Content-Type headers, while the new code only handles EUC-KR encoding and only when c.config.euckrResponse is true. This is a breaking change that will break proper decoding of non-EUC-KR encoded responses. The euckrResponse field also appears to be undefined in httpClientConfig.
| if c.config.euckrResponse { | ||
| var bufs bytes.Buffer | ||
| wr := transform.NewWriter(&bufs, korean.EUCKR.NewDecoder()) | ||
| wr.Write(buf) | ||
| wr.Close() | ||
| finalResponse = bufs.String() | ||
| } | ||
|
|
There was a problem hiding this comment.
The reference to c.config.euckrResponse will cause a compilation error because the euckrResponse field does not exist in the httpClientConfig struct. This field needs to be added to the httpClientConfig struct and there should be a corresponding WithEUCKRResponse option function to configure it, or this code should be removed if it's not intended to be part of this PR.
| if c.config.euckrResponse { | |
| var bufs bytes.Buffer | |
| wr := transform.NewWriter(&bufs, korean.EUCKR.NewDecoder()) | |
| wr.Write(buf) | |
| wr.Close() | |
| finalResponse = bufs.String() | |
| } |
| // Do issues a given HTTP request and returns the corresponding response. | ||
| // | ||
| // If the returned error is nil, the response contains a non-nil body, which the user is expected to close. | ||
| func (c *httpClient) Do(req *http.Request) (*http.Response, error) { |
There was a problem hiding this comment.
The WithPreHook and WithPostHook option functions are still defined in client_options.go and will successfully accept and store hooks in the config, but the hook execution logic has been removed from the Do method. This creates a silent failure where users can configure hooks but they will never be called, leading to confusing behavior. Either the option functions should be removed along with the implementation, or the implementation should be kept.
| if result.err == nil && result.response != nil && result.transport != nil { | ||
| pr.cacheWinningProtocol(addr, result.protocol, result.transport) | ||
| cancel() | ||
| return result.response, nil | ||
| } |
There was a problem hiding this comment.
The waitForRaceWinner function returns immediately upon finding a successful result and calls cancel(), but it doesn't wait for or clean up the losing transport. When HTTP/3 wins the race, the HTTP/2 goroutine may still be running, and when HTTP/2 wins, the HTTP/3 transport (if successfully created) is never closed. Consider draining the resultCh to receive and clean up the losing transport before returning.
| @@ -47,11 +50,6 @@ type HttpClient interface { | |||
| GetBandwidthTracker() bandwidth.BandwidthTracker | |||
| GetDialer() proxy.ContextDialer | |||
| GetTLSDialer() TLSDialerFunc | |||
|
|
|||
| AddPreRequestHook(hook PreRequestHookFunc) | |||
| AddPostResponseHook(hook PostResponseHookFunc) | |||
| ResetPreHooks() | |||
| ResetPostHooks() | |||
| } | |||
There was a problem hiding this comment.
The Close method implementation removes methods from the HttpClient interface (AddPreRequestHook, AddPostResponseHook, ResetPreHooks, ResetPostHooks) without deprecation notices or migration guidance. This is a breaking API change that could affect existing users of the library. Consider either keeping the interface methods and marking them as deprecated, or documenting this as a breaking change in release notes.
|
|
||
| // Close closes all connections of the underlying http client. | ||
| func (c *httpClient) Close() { | ||
| rt := c.Client.Transport.(*roundTripper) |
There was a problem hiding this comment.
The Close method uses a type assertion without checking for success, which will panic if the Transport is not a *roundTripper. Consider adding a check and handling the case where the type assertion fails, or at minimum documenting that this method requires the transport to be a *roundTripper.
| rt := c.Client.Transport.(*roundTripper) | |
| rt, ok := c.Client.Transport.(*roundTripper) | |
| if !ok || rt == nil { | |
| return | |
| } |
| } | ||
| } | ||
|
|
||
| transport, err := newRoundTripper(c.config.clientProfile, c.config.transportOptions, c.config.serverNameOverwrite, c.config.insecureSkipVerify, c.config.withRandomTlsExtensionOrder, c.config.forceHttp1, c.config.disableHttp3, c.config.enableProtocolRacing, c.config.certificatePins, c.config.badPinHandler, c.config.disableIPV6, c.config.disableIPV4, c.bandwidthTracker, dialer) |
There was a problem hiding this comment.
The WithDialContext option function is still defined and will successfully set config.dialContext, but the implementation that uses this field has been removed from both buildFromConfig and applyProxy. This creates a silent failure where users can configure a custom dial context but it will never be used. Either the option function should be removed, or the implementation should be restored.
This pull request makes several significant changes to the
tls_clientpackage, primarily focused on simplifying theHttpClientinterface and implementation, improving connection management, and refining protocol racing logic. The most notable changes include the removal of request/response hooks, the addition of aClose()method for better resource management, and updates to protocol transport caching.Interface and Hook Removal
HttpClient: The interface and implementation no longer support adding, resetting, or executing pre-request and post-response hooks, simplifying the client and reducing thread-safety complexity. (client.go[1] [2] [3] [4]Connection and Resource Management
Close()method toHttpClient: Introduced aClose()method that properly shuts down underlying HTTP/2 and HTTP/3 transports, allowing for explicit resource cleanup. (client.go[1] [2]Protocol Racing and Transport Caching
racer.go[1] [2] [3] [4] [5]Charset Handling
golang.org/x/text/encoding/koreanpackage. (client.go[1] [2] [3]Miscellaneous
DialContextand its associated logic has been removed, further simplifying client configuration and proxy handling. (client.go[1] [2] [3]These changes collectively streamline the
tls_clientcodebase, making it easier to maintain and less error-prone, while also improving resource management and protocol handling.