Repository: cloudwego/hertz
Pull request #989 (2023-11-06 06:57:07)

optimize: trace doFinish multiple times
#### 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
-->
optimize
#### 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:
-->
- [ ] This PR title match the format: \<type\>(optional scope): \<description\>
- [ ] 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.
优化trace中 dofinish 执行多次的情况

#### (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: 优化trace中 dofinish 执行多次的情况
zh(optional): 用户使用短连接的时候，trace.DoFinish 会执行多次，导致上报重复的情况

#### (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/989?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) Report
Attention: `2 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`e02c162`)](https://app.codecov.io/gh/cloudwego/hertz/commit/e02c16214bb98372093a3c7817287c208801de7f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 81.86% compared to head [(`bb77e7a`)](https://app.codecov.io/gh/cloudwego/hertz/pull/989?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 81.80%.

> :exclamation: Current head bb77e7a differs from pull request most recent head 0639f55. Consider uploading reports for the commit 0639f55 to get more accurate results

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


```diff
@@             Coverage Diff             @@
##           develop     #989      +/-   ##
===========================================
- Coverage    81.86%   81.80%   -0.07%     
===========================================
  Files           98       98              
  Lines         9932     9932              
===========================================
- Hits          8131     8125       -6     
- Misses        1311     1316       +5     
- Partials       490      491       +1     
```


| [Files](https://app.codecov.io/gh/cloudwego/hertz/pull/989?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) | Coverage Δ | |
|---|---|---|
| [pkg/protocol/http1/server.go](https://app.codecov.io/gh/cloudwego/hertz/pull/989?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego#diff-cGtnL3Byb3RvY29sL2h0dHAxL3NlcnZlci5nbw==) | `72.24% <0.00%> (-0.96%)` | :arrow_down: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/cloudwego/hertz/pull/989/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/cloudwego/hertz/pull/989?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).


Patch:
diff --git a/.gitattributes b/.gitattributes
new file mode 100644
index 0000000..8c2820f
--- /dev/null
+++ b/.gitattributes
@@ -0,0 +1,7 @@
+# Handle line endings automatically for files detected as text
+# and leave all files detected as binary untouched.
+* text=auto eol=lf
+
+# Force bash scripts to always use LF line endings so that if a repo is accessed
+# in Unix via a file share from Windows, the scripts will work.
+*.sh text eol=lf
diff --git a/.github/workflows/vulncheck.yml b/.github/workflows/vulncheck.yml
new file mode 100644
index 0000000..9b88498
--- /dev/null
+++ b/.github/workflows/vulncheck.yml
@@ -0,0 +1,35 @@
+name: Run govulncheck
+
+on:
+  push:
+    branches:
+      - develop
+    paths:
+      - "**"
+      - "!**.md"
+  pull_request:
+    paths:
+      - "**"
+      - "!**.md"
+
+jobs:
+  vulncheck-check:
+    runs-on: ubuntu-latest
+    env:
+      GO111MODULE: on
+    steps:
+      - name: Fetch Repository
+        uses: actions/checkout@v4
+
+      - name: Install Go
+        uses: actions/setup-go@v4
+        with:
+          go-version: "stable"
+          check-latest: true
+          cache: false
+
+      - name: Install Govulncheck
+        run: go install golang.org/x/vuln/cmd/govulncheck@latest
+
+      - name: Run Govulncheck
+        run: govulncheck ./...
diff --git a/pkg/protocol/http1/server.go b/pkg/protocol/http1/server.go
index 66cb01e..ef19cd7 100644
--- a/pkg/protocol/http1/server.go
+++ b/pkg/protocol/http1/server.go
@@ -369,11 +369,6 @@ func (s Server) Serve(c context.Context, conn network.Conn) (err error) {
 			return
 		}
 
-		// general case
-		if s.EnableTrace {
-			traceCtl.DoFinish(cc, ctx, err)
-		}
-
 		if connectionClose {
 			return errShortConnection
 		}
@@ -382,6 +377,10 @@ func (s Server) Serve(c context.Context, conn network.Conn) (err error) {
 		if s.IdleTimeout == 0 {
 			return
 		}
+		// general case
+		if s.EnableTrace {
+			traceCtl.DoFinish(cc, ctx, err)
+		}
 
 		ctx.ResetWithoutConn()
 	}


Test patch:
diff --git a/pkg/protocol/http1/server_test.go b/pkg/protocol/http1/server_test.go
index e34bb3f..5f8f782 100644
--- a/pkg/protocol/http1/server_test.go
+++ b/pkg/protocol/http1/server_test.go
@@ -359,6 +359,52 @@ func TestExpect100ContinueHandler(t *testing.T) {
 	assert.DeepEqual(t, "", string(response.Body()))
 }
 
+type mockController struct {
+	FinishTimes int
+}
+
+func (m *mockController) Append(col tracer.Tracer) {}
+
+func (m *mockController) DoStart(ctx context.Context, c *app.RequestContext) context.Context {
+	return ctx
+}
+
+func (m *mockController) DoFinish(ctx context.Context, c *app.RequestContext, err error) {
+	m.FinishTimes++
+}
+
+func (m *mockController) HasTracer() bool { return true }
+
+func (m *mockController) reset() { m.FinishTimes = 0 }
+
+func TestTraceDoFinishTimes(t *testing.T) {
+	server := &Server{}
+	server.eventStackPool = pool
+	server.EnableTrace = true
+	reqCtx := &app.RequestContext{}
+	controller := &mockController{}
+	server.Core = &mockCore{
+		ctxPool: &sync.Pool{New: func() interface{} {
+			ti := traceinfo.NewTraceInfo()
+			ti.Stats().SetLevel(2)
+			reqCtx.SetTraceInfo(&mockTraceInfo{ti})
+			return reqCtx
+		}},
+		controller: controller,
+	}
+	// for disableKeepAlive case
+	server.DisableKeepalive = true
+	err := server.Serve(context.TODO(), mock.NewConn("GET /aaa HTTP/1.1\nHost: foobar.com\n\n"))
+	assert.True(t, errors.Is(err, errs.ErrShortConnection))
+	assert.DeepEqual(t, 1, controller.FinishTimes)
+	// for IdleTimeout==0 case
+	server.IdleTimeout = 0
+	controller.reset()
+	err = server.Serve(context.TODO(), mock.NewConn("GET /aaa HTTP/1.1\nHost: foobar.com\n\n"))
+	assert.True(t, errors.Is(err, errs.ErrShortConnection))
+	assert.DeepEqual(t, 1, controller.FinishTimes)
+}
+
 type mockCore struct {
 	ctxPool     *sync.Pool
 	controller  tracer.Controller