Repository: cloudwego/hertz
Pull request #1295 (2025-03-13 07:23:40)

fix(client): add cleanup for https HostClient map
#### What type of PR is this?
<!--
Add one of the following kinds:

build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
docs: Documentation only changes
feat: A new feature
optimize: A new optimization
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
style: Changes that do not affect the meaning of the code (white space, formatting, missing semi-colons, etc)
test: Adding missing tests or correcting existing tests
chore: Changes to the build process or auxiliary tools and libraries such as documentation generation
-->
fix

#### Check the PR title.
<!--
The description of the title will be attached in Release Notes, 
so please describe it from user-oriented, what this PR does / why we need it.
Please check your PR title with the below requirements:
-->
- [x] This PR title match the format: \<type\>(optional scope): \<description\>
- [x] The description of this PR title is user-oriented and clear enough for others to understand.
- [ ] Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)


#### (Optional) Translate the PR title into Chinese.


#### (Optional) More detailed description for this PR(en: English/zh: Chinese).
<!--
Provide more detailed info for review(e.g., it's recommended to provide perf data if this is a perf type PR).
-->
en:
zh(optional):

#### (Optional) Which issue(s) this PR fixes:
<!--
Automatically closes linked issue when PR is merged.
Eg: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

#### (Optional) The PR that updates user documentation:
<!--
If the current PR requires user awareness at the usage level, please submit a PR to update user docs. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)
-->

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/cloudwego/hertz/pull/1295?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) Report
Attention: Patch coverage is `92.85714%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 72.23%. Comparing base [(`7e5e773`)](https://app.codecov.io/gh/cloudwego/hertz/commit/7e5e773cdf6ada279c45fba7f51d65aec6f6a6d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) to head [(`77ac718`)](https://app.codecov.io/gh/cloudwego/hertz/commit/77ac718448659fc7851f739878989e07cf6dd1b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).
> Report is 3 commits behind head on develop.

| [Files with missing lines](https://app.codecov.io/gh/cloudwego/hertz/pull/1295?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) | Patch % | Lines |
|---|---|---|
| [pkg/app/client/client.go](https://app.codecov.io/gh/cloudwego/hertz/pull/1295?src=pr&el=tree&filepath=pkg%2Fapp%2Fclient%2Fclient.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego#diff-cGtnL2FwcC9jbGllbnQvY2xpZW50Lmdv) | 92.85% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/cloudwego/hertz/pull/1295?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           develop    #1295      +/-   ##
===========================================
+ Coverage    72.20%   72.23%   +0.03%     
===========================================
  Files          138      138              
  Lines        16137    16144       +7     
===========================================
+ Hits         11652    11662      +10     
+ Misses        3890     3887       -3     
  Partials       595      595              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/cloudwego/hertz/pull/1295?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).

<details><summary>🚀 New features to boost your workflow: </summary>

- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
xiaost: I'm testing AI code review, ignore the suggestions below if you think it's ok.


---
Here are the areas for improvement:

- In `cleanHostClients` function, the logic for selecting the map (`c.m` or `c.ms`) and then iterating through it is slightly repetitive. You could extract the map selection logic outside the loop for better readability.

- The `cleaner` goroutine is launched every time `do` is called when `startCleaner` is true.  Consider whether this might lead to launching multiple cleaners if there are concurrent requests for new hosts. It's better to ensure only one cleaner goroutine is running at a time, possibly by using a channel or atomic boolean flag to track if the cleaner is already running.

- In `TestCleanHostClients`, while the test covers different scenarios, consider adding a test case to verify that the `Close()` method on `v.(io.Closer)` is actually called when `ShouldRemove()` returns true and `v` implements `io.Closer`. You have added mock for this in the test case, which is good, but explicitly asserting `m.closed` is true after `cleanHostClients` is called will make the test more robust and directly verify the closing behavior. You are already asserting this. Good.

- In `TestClientProxyWithNetpollDialer`, the test case ` {false, true}` is duplicated. It should be `{true, true}` for complete coverage of the boolean combinations. This has been fixed in the patch, which is good.

Patch:
diff --git a/pkg/app/client/client.go b/pkg/app/client/client.go
index e750096..09a371a 100644
--- a/pkg/app/client/client.go
+++ b/pkg/app/client/client.go
@@ -532,7 +532,7 @@ func (c *Client) do(ctx context.Context, req *protocol.Request, resp *protocol.R
 	c.mLock.Unlock()
 
 	if startCleaner {
-		go c.mCleaner()
+		go c.cleaner(isTLS)
 	}
 
 	return hc.Do(ctx, req, resp)
@@ -550,21 +550,25 @@ func (c *Client) CloseIdleConnections() {
 	c.mLock.Unlock()
 }
 
-func (c *Client) mCleaner() {
+func (c *Client) cleaner(isTLS bool) {
 	for {
 		time.Sleep(10 * time.Second)
-		if c.mClean() {
+		if c.cleanHostClients(isTLS) {
 			break
 		}
 	}
 }
 
-func (c *Client) mClean() bool {
+func (c *Client) cleanHostClients(isTLS bool) bool {
 	c.mLock.Lock()
 	defer c.mLock.Unlock()
-	for k, v := range c.m {
+	m := c.m
+	if isTLS {
+		m = c.ms
+	}
+	for k, v := range m {
 		if v.ShouldRemove() {
-			delete(c.m, k)
+			delete(m, k)
 			if f, ok := v.(io.Closer); ok {
 				err := f.Close()
 				if err != nil {
@@ -573,7 +577,7 @@ func (c *Client) mClean() bool {
 			}
 		}
 	}
-	return len(c.m) == 0
+	return len(m) == 0
 }
 
 func (c *Client) SetClientFactory(cf suite.ClientFactory) {


Test patch:
diff --git a/pkg/app/client/client_test.go b/pkg/app/client/client_test.go
index 7c13c65..5e182f6 100644
--- a/pkg/app/client/client_test.go
+++ b/pkg/app/client/client_test.go
@@ -76,6 +76,7 @@ import (
 	"github.com/cloudwego/hertz/pkg/network/dialer"
 	"github.com/cloudwego/hertz/pkg/network/standard"
 	"github.com/cloudwego/hertz/pkg/protocol"
+	"github.com/cloudwego/hertz/pkg/protocol/client"
 	"github.com/cloudwego/hertz/pkg/protocol/consts"
 	"github.com/cloudwego/hertz/pkg/protocol/http1"
 	"github.com/cloudwego/hertz/pkg/protocol/http1/req"
@@ -157,7 +158,7 @@ func TestCloseIdleConnections(t *testing.T) {
 		t.Errorf("expected 0 conns got %d", conns)
 	}
 
-	c.mClean()
+	c.cleanHostClients(false)
 
 	func() {
 		c.mLock.Lock()
@@ -1754,7 +1755,7 @@ func TestClientProxyWithNetpollDialer(t *testing.T) {
 		{false, false},
 		{true, false},
 		{false, true},
-		{false, true},
+		{true, true},
 	}
 	for _, testCase := range testCases {
 		httpsSite := testCase.httpsSite
@@ -2452,3 +2453,126 @@ func TestClientRetryErr(t *testing.T) {
 		l.Unlock()
 	})
 }
+
+type mockHostClient struct {
+	shouldRemove bool
+	closed       bool
+}
+
+func (m *mockHostClient) Do(ctx context.Context, req *protocol.Request, resp *protocol.Response) error {
+	return nil
+}
+
+func (m *mockHostClient) SetDynamicConfig(dc *client.DynamicConfig) {
+}
+
+func (m *mockHostClient) CloseIdleConnections() {
+}
+
+func (m *mockHostClient) ShouldRemove() bool {
+	return m.shouldRemove
+}
+
+func (m *mockHostClient) ConnectionCount() int {
+	return 0
+}
+
+func (m *mockHostClient) Close() error {
+	m.closed = true
+	return nil
+}
+
+func TestCleanHostClients(t *testing.T) {
+	tests := []struct {
+		name         string
+		isTLS        bool
+		initMap      map[string]*mockHostClient
+		expectedKeys []string
+		shouldClose  bool
+		expectedRes  bool
+	}{
+		{
+			name:  "Remove item from c.m",
+			isTLS: false,
+			initMap: map[string]*mockHostClient{
+				"google.com": {shouldRemove: true},
+			},
+			expectedKeys: []string{},
+			shouldClose:  true,
+			expectedRes:  true,
+		},
+		{
+			name:  "Remove item from c.ms",
+			isTLS: true,
+			initMap: map[string]*mockHostClient{
+				"google.com": {shouldRemove: true},
+			},
+			expectedKeys: []string{},
+			shouldClose:  true,
+			expectedRes:  true,
+		},
+		{
+			name:  "Do not remove non-removable client",
+			isTLS: false,
+			initMap: map[string]*mockHostClient{
+				"google.com": {shouldRemove: false},
+			},
+			expectedKeys: []string{"google.com"},
+			shouldClose:  false,
+			expectedRes:  false,
+		},
+		{
+			name:         "Empty map",
+			isTLS:        false,
+			initMap:      map[string]*mockHostClient{},
+			expectedKeys: []string{},
+			shouldClose:  false,
+			expectedRes:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cli := &Client{
+				mLock: sync.Mutex{},
+				m:     map[string]client.HostClient{},
+				ms:    map[string]client.HostClient{},
+			}
+
+			if tt.isTLS {
+				for k, v := range tt.initMap {
+					cli.ms[k] = v
+				}
+			} else {
+				for k, v := range tt.initMap {
+					cli.m[k] = v
+				}
+			}
+
+			result := cli.cleanHostClients(tt.isTLS)
+
+			var resultMap map[string]client.HostClient
+			if tt.isTLS {
+				resultMap = cli.ms
+			} else {
+				resultMap = cli.m
+			}
+
+			var keys []string
+			for k := range resultMap {
+				keys = append(keys, k)
+			}
+			assert.Assert(t, len(tt.expectedKeys) == len(keys))
+
+			for _, v := range tt.initMap {
+				if v.shouldRemove {
+					assert.Assert(t, v.closed, "Expected client to be closed")
+				} else {
+					assert.Assert(t, !v.closed, "Client should not be closed")
+				}
+			}
+
+			assert.Assert(t, tt.expectedRes == result)
+		})
+	}
+}