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

feat: ReleaseRequest support async release #608

Closed
wants to merge 1 commit into from

Conversation

wzekin
Copy link
Contributor

@wzekin wzekin commented Feb 13, 2023

What type of PR is this?

feat

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.

为 ReleaseRequest 添加 async release 的支持

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

en:

Question.
In h1, client.Do is guaranteed to send the Request and then return. But in h2, client.Do will return directly after sending Request Header and receiving Response Header, so if we call ReleaseRequest directly after client.Do returns, Data Race will occur.

Solution.
1. add sendDone field and IsSendEnd, WaitSendEnd, SetSendEnd(internal) functions to Request to mark whether the request is sent or not
2. add a test at ReleaseRequest to check if the request is not sent, and async wait until it is finished

zh(optional):

问题:
在 h1 中,client.Do 保证发送完 Request 再返回。但在 h2 中,client.Do 会在发送完 Request Header 且接收到 Response Header 后直接返回,这时如果在 client.Do 返回后直接调用 ReleaseRequest 就会出现 Data Race。

解决方案:
1. 在Request 添加sendDone字段以及 IsSendEnd, WaitSendEnd, SetSendEnd(internal)函数,用于标记请求是否发送完成
2. 在 ReleaseRequest 中添加检测,如果未发送完成,则等待发送结束之后再释放 Request

Which issue(s) this PR fixes:

@wzekin wzekin requested review from a team as code owners February 13, 2023 07:46
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Base: 72.07% // Head: 71.91% // Decreases project coverage by -0.16% ⚠️

Coverage data is based on head (148faac) compared to base (59ae2bd).
Patch coverage: 4.54% of modified lines in pull request are covered.

❗ Current head 148faac differs from pull request most recent head 3a58309. Consider uploading reports for the commit 3a58309 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #608      +/-   ##
===========================================
- Coverage    72.07%   71.91%   -0.16%     
===========================================
  Files           96       96              
  Lines         9389     9411      +22     
===========================================
+ Hits          6767     6768       +1     
- Misses        2189     2210      +21     
  Partials       433      433              
Impacted Files Coverage Δ
pkg/protocol/request.go 79.55% <4.54%> (-4.30%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -523,11 +523,18 @@ func (c *HostClient) doNonNilReqResp(req *protocol.Request, resp *protocol.Respo
}
zw := c.acquireWriter(conn)

// need this? in fact, IsSendEnd always return true in h1
sendDone := make(chan struct{})
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no need in h1

@wzekin wzekin force-pushed the feat/async_release branch from 77bbe08 to 3a58309 Compare February 22, 2023 11:01
@@ -814,6 +818,36 @@ func (req *Request) SetConnectionClose() {
req.Header.SetConnectionClose(true)
}

// IsSendEnd return true if the client has sent all data of this request
func (req *Request) IsSendEnd() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps change it to private method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is used by http2

}

// WaitSendEnd will wait until the client has sent all data of this request
func (req *Request) WaitSendEnd() {
Copy link
Member

Choose a reason for hiding this comment

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

AA

// SetSendDone set a sendDone channel to Request
//
// NOTE: It is an internal function. You should not use it.
func (req *Request) SetSendDone(sendDone <-chan struct{}) {
Copy link
Member

Choose a reason for hiding this comment

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

AA

@kolinfluence
Copy link

possible to expedite this merge?

@xiaost xiaost closed this Jan 16, 2025
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