Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: standard conn return non nil err with data #829

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

Duslia
Copy link
Member

@Duslia Duslia commented Jun 21, 2023

What type of PR is this?

fix

Check the PR title.

  • 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.

(Optional) Translate the PR title into Chinese.

修复标准库连接同时返回了数据和错误时没有正确处理数据的 bug

(Optional) More detail description for this PR(en: English/zh: Chinese).

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:

#827

(Optional) The PR that updates user documentation:

@Duslia Duslia requested review from a team as code owners June 21, 2023 09:20
@Duslia Duslia changed the title test: standard conn return non nil err with data fix: standard conn return non nil err with data Jun 21, 2023
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 95.45% and project coverage change: +0.02 🎉

Comparison is base (f4f193d) 75.70% compared to head (bfd486b) 75.73%.

❗ Current head bfd486b differs from pull request most recent head e5a2d86. Consider uploading reports for the commit e5a2d86 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #829      +/-   ##
===========================================
+ Coverage    75.70%   75.73%   +0.02%     
===========================================
  Files           97       97              
  Lines         9495     9510      +15     
===========================================
+ Hits          7188     7202      +14     
- Misses        1857     1858       +1     
  Partials       450      450              
Impacted Files Coverage Δ
pkg/network/standard/connection.go 81.62% <95.45%> (+0.55%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Duslia Duslia force-pushed the fix/read_n_with_non_nil_err branch 2 times, most recently from 19956a4 to bdd78f4 Compare June 26, 2023 08:57
@welkeyever
Copy link
Member

Add some test cases for this issue?

@Duslia Duslia force-pushed the fix/read_n_with_non_nil_err branch 3 times, most recently from 00863be to a7271bd Compare June 27, 2023 11:32
@Duslia Duslia force-pushed the fix/read_n_with_non_nil_err branch from a7271bd to e5a2d86 Compare June 28, 2023 03:47
@Duslia Duslia enabled auto-merge (squash) June 30, 2023 03:18
@Duslia Duslia merged commit b737524 into cloudwego:develop Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants