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: stream client not close the conn when ConnectionClose is true #702

Merged
merged 2 commits into from
Apr 7, 2023

Conversation

wzekin
Copy link
Contributor

@wzekin wzekin commented Apr 3, 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.

(Optional) Translate the PR title into Chinese.

client 在流式场景下,当 ConnectionClose 为 true 的时候,没有关闭链接

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

en:

  1. client uses streaming to receive resp, and is set to Connection: close
  2. when the bodyStream is closed, the client does not determine if the connection is currently closed, but simply reuses the connection
  3. the connection is closed by the server because of Connection: close
  4. when the client sends a second request using this connection, it will report an error directly because the connection is closed

zh(optional): 存在这样一个场景:

  1. client 使用流式接收 resp,且被设置了 Connection: close
  2. 当 bodyStream 被关闭的时候,client 不会判断当前是否已经关闭了连接,而是直接将连接复用
  3. 连接因为 Connection: close 被 server 关闭
  4. 当 client 使用这个连接发送第二个请求的时候,会因为连接被关闭直接报错

Which issue(s) this PR fixes:

@wzekin wzekin requested review from a team as code owners April 3, 2023 06:45
welkeyever
welkeyever previously approved these changes Apr 3, 2023
Copy link
Member

@welkeyever welkeyever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 42.85% and project coverage change: -0.03 ⚠️

Comparison is base (be62800) 75.72% compared to head (b19384c) 75.70%.

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #702      +/-   ##
===========================================
- Coverage    75.72%   75.70%   -0.03%     
===========================================
  Files           96       96              
  Lines         9451     9455       +4     
===========================================
+ Hits          7157     7158       +1     
- Misses        1848     1849       +1     
- Partials       446      448       +2     
Impacted Files Coverage Δ
pkg/protocol/http1/client.go 59.59% <42.85%> (-0.26%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@wzekin wzekin force-pushed the fix/stream_conn_close branch from dd494dc to 5fec78f Compare April 7, 2023 02:55
@wzekin wzekin merged commit 8e9fce9 into cloudwego:develop Apr 7, 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.

5 participants