Repository: cloudwego/hertz
Pull request #829 (2023-06-21 09:20:01)

fix: standard conn return non nil err with data
#### 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.
- [x] 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.
修复标准库连接同时返回了数据和错误时没有正确处理数据的 bug

#### (Optional) More detail description for this PR(en: English/zh: Chinese).
<!--
Provide more detailed info for review. If it is a perf type PR, perf data is suggested to give.
-->
en: When both data and an error are returned by a connection, prioritizing the data over the error is recommended, consistent with the `io.Reader` best practices.
1. `standard Conn` adds an `err` field to store the error that occurs when `conn.Read` returns n > 0 & err != nil.
2. When `conn.Read` returns n > 0 & err != nil, `fill` returns directly regardless of how much data has been filled. The upper layer determines whether the data is complete by using the `Conn.Len()` method. When calling `fill(1)`, there is no need for judgment, because `fill` can only return 0, err.
3. Before calling `conn.read` in `fill`, check whether the last `fill` returned an error. If it did, directly return that error to the upper layer.
4. After calling `fill`, the upper layer uses `c.Len()` to determine whether it has filled enough data. If it has, return the data from this call. If it has not, return the data already read and the error together.

zh(optional): 当连接同时返回了数据和错误时，优先处理数据而不是错误，和 `io.Reader` 推荐做法一致。具体改动如下：
1. `standard Conn` 增加 err 字段，用于存储 `conn.Read` 同时返回 n > 0 & err != nil 时的 err
2. 当 `conn.Read` 同时返回 n > 0 & err != nil 时，`fill` 无论填了多少数据都直接返回，上层通过 `Conn.Len()` 方法判断数据是否填充完整；调用 fill(1) 时不需要判断，因为 fill 只可能 return 0, err。
3. `fill` 调用 `Conn.Read` 之前之前先判断上次 `fill` 是否返回了 err，如果返回了，则直接将该 err 返回给上层。
4. 上层调用 `fill` 之后使用 `c.Len()` 判断是否填充了足量的数据，如果够，则返回这次的数据；如果不够，则把已经读的数据和 err 一并返回

#### (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)`.
-->
https://github.com/cloudwego/hertz/issues/827
#### (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/829?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) Report
Patch coverage: **`66.66`**% and project coverage change: **`-0.02`** :warning:
> Comparison is base [(`f4f193d`)](https://app.codecov.io/gh/cloudwego/hertz/commit/f4f193d908a1f10a4898a07ced819ab02784bb24?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 75.70% compared to head [(`61b280d`)](https://app.codecov.io/gh/cloudwego/hertz/pull/829?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 75.68%.

> :exclamation: Current head 61b280d differs from pull request most recent head 9c24cd3. Consider uploading reports for the commit 9c24cd3 to get more accurate results

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


```diff
@@             Coverage Diff             @@
##           develop     #829      +/-   ##
===========================================
- Coverage    75.70%   75.68%   -0.02%     
===========================================
  Files           97       97              
  Lines         9495     9498       +3     
===========================================
+ Hits          7188     7189       +1     
- Misses        1857     1858       +1     
- Partials       450      451       +1     
```


| [Impacted Files](https://app.codecov.io/gh/cloudwego/hertz/pull/829?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) | Coverage Δ | |
|---|---|---|
| [pkg/network/standard/connection.go](https://app.codecov.io/gh/cloudwego/hertz/pull/829?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego#diff-cGtnL25ldHdvcmsvc3RhbmRhcmQvY29ubmVjdGlvbi5nbw==) | `80.62% <66.66%> (-0.45%)` | :arrow_down: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/cloudwego/hertz/pull/829?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).

welkeyever: Add some test cases for this issue? 

Patch:
diff --git a/pkg/network/standard/connection.go b/pkg/network/standard/connection.go
index 65fd08f..e38ee9e 100644
--- a/pkg/network/standard/connection.go
+++ b/pkg/network/standard/connection.go
@@ -46,6 +46,8 @@ type Conn struct {
 	outputBuffer *linkBuffer
 	caches       [][]byte // buf allocated by Next when cross-package, which should be freed when release
 	maxSize      int      // history max malloc size
+
+	err error
 }
 
 func (c *Conn) ToHertzError(err error) error {
@@ -91,6 +93,8 @@ func (c *Conn) Read(b []byte) (l int, err error) {
 	// If left buffer size is less than block4k, first Peek(1) to fill the buffer.
 	// Then copy min(c.Len, len(b)) to b.
 	if len(b) <= block4k {
+		// If c.fill(1) return err, conn.Read must return 0, err. So there is no need
+		// to check c.Len
 		err = c.fill(1)
 		if err != nil {
 			return 0, err
@@ -293,6 +297,9 @@ func (c *Conn) handleTail() {
 	c.inputBuffer.write.Reset()
 }
 
+// Peek returns the next n bytes without advancing the reader. The bytes stop
+// being valid at the next read call. If Peek returns fewer than n bytes, it
+// also returns an error explaining why the read is short.
 func (c *Conn) Peek(i int) (p []byte, err error) {
 	node := c.inputBuffer.read
 	// fill the inputBuffer so that there is enough data
@@ -301,10 +308,15 @@ func (c *Conn) Peek(i int) (p []byte, err error) {
 		return
 	}
 
+	if c.Len() < i {
+		i = c.Len()
+		err = c.readErr()
+	}
+
 	l := node.Len()
 	// Enough data in a single node, so that just return the slice of the node.
 	if l >= i {
-		return node.buf[node.off : node.off+i], nil
+		return node.buf[node.off : node.off+i], err
 	}
 
 	// not enough data in a signal node
@@ -314,12 +326,12 @@ func (c *Conn) Peek(i int) (p []byte, err error) {
 	} else {
 		p = make([]byte, i)
 	}
-	err = c.peekBuffer(i, p)
+	c.peekBuffer(i, p)
 	return p, err
 }
 
 // peekBuffer loads the buf with data of size i without moving read pointer.
-func (c *Conn) peekBuffer(i int, buf []byte) error {
+func (c *Conn) peekBuffer(i int, buf []byte) {
 	l, pIdx, node := 0, 0, c.inputBuffer.read
 	for ack := i; ack > 0; ack = ack - l {
 		l = node.Len()
@@ -331,7 +343,6 @@ func (c *Conn) peekBuffer(i int, buf []byte) error {
 		}
 		node = node.next
 	}
-	return nil
 }
 
 // next loads the buf with data of size i with moving read pointer.
@@ -345,12 +356,22 @@ func (c *Conn) next(length int, b []byte) error {
 }
 
 // fill loads more data than size i, otherwise it will block read.
+// NOTE: fill may fill data less than i and store err in Conn.err
+// when last read returns n > 0 and err != nil. So after calling
+// fill, it is necessary to check whether c.Len() > i
 func (c *Conn) fill(i int) (err error) {
 	// Check if there is enough data in inputBuffer.
 	if c.Len() >= i {
 		return nil
 	}
-
+	// check whether conn has returned err before.
+	if err = c.readErr(); err != nil {
+		if c.Len() > 0 {
+			c.err = err
+			return nil
+		}
+		return
+	}
 	node := c.inputBuffer.write
 	node.buf = node.buf[:cap(node.buf)]
 	left := cap(node.buf) - node.malloc
@@ -374,15 +395,22 @@ func (c *Conn) fill(i int) (err error) {
 	i -= c.Len()
 	node = c.inputBuffer.write
 	node.buf = node.buf[:cap(node.buf)]
+
 	// Circulate reading data so that the node holds enough data
 	for i > 0 {
 		n, err := c.c.Read(c.inputBuffer.write.buf[node.malloc:])
+		if n > 0 {
+			node.malloc += n
+			c.inputBuffer.len += n
+			i -= n
+			if err != nil {
+				c.err = err
+				return nil
+			}
+		}
 		if err != nil {
 			return err
 		}
-		node.malloc += n
-		c.inputBuffer.len += n
-		i -= n
 	}
 	return nil
 }
@@ -533,6 +561,12 @@ func (c *Conn) HandleSpecificError(err error, rip string) (needIgnore bool) {
 	return false
 }
 
+func (c *Conn) readErr() error {
+	err := c.err
+	c.err = nil
+	return err
+}
+
 func (c *TLSConn) Handshake() error {
 	return c.c.(network.ConnTLSer).Handshake()
 }


Test patch:
diff --git a/pkg/network/standard/connection_test.go b/pkg/network/standard/connection_test.go
index 6fcc456..fa950f3 100644
--- a/pkg/network/standard/connection_test.go
+++ b/pkg/network/standard/connection_test.go
@@ -274,9 +274,10 @@ func TestHandleSpecificError(t *testing.T) {
 }
 
 type mockConn struct {
-	buffer     bytes.Buffer
-	localAddr  net.Addr
-	remoteAddr net.Addr
+	buffer        bytes.Buffer
+	localAddr     net.Addr
+	remoteAddr    net.Addr
+	readReturnErr bool
 }
 
 func (m *mockConn) Handshake() error {
@@ -292,17 +293,21 @@ func (m mockConn) Read(b []byte) (n int, err error) {
 	for i := 0; i < length; i++ {
 		b[i] = 0
 	}
+
+	if m.readReturnErr {
+		err = io.EOF
+	}
 	if length > 8192 {
-		return 8192, nil
+		return 8192, err
 	}
 	if len(b) < 1024 {
-		return 100, nil
+		return 100, err
 	}
 	if len(b) < 5000 {
-		return 4096, nil
+		return 4096, err
 	}
 
-	return 4099, nil
+	return 4099, err
 }
 
 func (m *mockConn) Write(b []byte) (n int, err error) {
@@ -378,3 +383,17 @@ func TestConnSetFinalizer(t *testing.T) {
 
 	assert.DeepEqual(t, uint32(2), atomic.LoadUint32(&release_count))
 }
+
+func TestFillReturnErrAndN(t *testing.T) {
+	c := &mockConn{
+		readReturnErr: true,
+	}
+	conn := newConn(c, 4099)
+	b, err := conn.Peek(4099)
+	assert.Nil(t, err)
+	assert.DeepEqual(t, len(b), 4099)
+	conn.Skip(10)
+	b, err = conn.Peek(4099)
+	assert.DeepEqual(t, err, io.EOF)
+	assert.DeepEqual(t, len(b), 4089)
+}