http2: perform connection health check #55
Conversation
|
This PR (HEAD: bc0d6c6) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Brad Fitzpatrick: Patch Set 1: (11 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Chao Xu: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: 840ca83) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
caesarxuchao
force-pushed
the
ping-connection
branch
from
840ca83 to
b2e6f87
Compare
|
This PR (HEAD: b2e6f87) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Chao Xu: Patch Set 2: Hi Brad, Thank you for the review. I modified the periodic ping behavior to start only after the readloop has been idle for a while, and to stop once new frame is received in the readloop. I have also addressed other comments. PTAL. Thank you. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Chao Xu: Patch Set 3:
I will be OOO until Oct. 30. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Brad Fitzpatrick: Patch Set 3: (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: 331a10c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Brad Fitzpatrick: Patch Set 4: (6 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Daniel Smith: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Chao Xu: Patch Set 4: (4 comments)
Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: 5058c86) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Andrew Bonventre: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Michael Fraenkel: Patch Set 5: (4 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Michael Fraenkel: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Chao Xu: Patch Set 5: Thank you for the reviews. I'll address them the next week. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: 231a5fe) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Chao Xu: Patch Set 5: (4 comments) @michael, I managed to simplify the code a lot. See the inline replies. PTAL. Thank you for the review! Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Michael Fraenkel: Patch Set 6: (8 comments) Glad there was some simplification to this. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: ee11e85) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Chao Xu: Patch Set 6: (7 comments) Thanks, Michael. PTAL. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Chao Xu: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Michael Fraenkel: Patch Set 7: (2 comments) Just a few minor changes. Otherwise it looks much better. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
This PR (HEAD: bce9e97) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
After the connection has been idle for a while, periodic pings are sent over the connection to check its health. Unhealthy connection is closed and removed from the connection pool. Fixes golang/go#31643
caesarxuchao
force-pushed
the
ping-connection
branch
from
bce9e97 to
36607fe
Compare
|
This PR (HEAD: 36607fe) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/198040 to see it. Tip: You can toggle comments from me using the |
|
Message from Chao Xu: Patch Set 16: (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: (16 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: (10 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: (5 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Gobot Gobot: Patch Set 16: TryBots beginning. Status page: https://farmer.golang.org/try?commit=74d0d8c3 Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Russ Cox: Patch Set 16: If the trybots come back happy, this is OK to submit. In the future, please mark resolved comments resolved, using Done or the checkbox. Thanks. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Gobot Gobot: Patch Set 16: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Andrew Bonventre: Patch Set 17: Patch Set 16 was rebased Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Andrew Bonventre: Patch Set 17: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Gobot Gobot: Patch Set 17: TryBots beginning. Status page: https://farmer.golang.org/try?commit=f8b7a1fa Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
|
Message from Gobot Gobot: Patch Set 17: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/198040. |
After the connection has been idle for a while, periodic pings are sent over the connection to check its health. Unhealthy connection is closed and removed from the connection pool. Fixes golang/go#31643 Change-Id: Idbbc9cb2d3e26c39f84a33e945e482d41cd8583c GitHub-Last-Rev: 36607fe GitHub-Pull-Request: #55 Reviewed-on: https://go-review.googlesource.com/c/net/+/198040 Run-TryBot: Andrew Bonventre <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Russ Cox <[email protected]>
|
This PR is being closed because golang.org/cl/198040 has been merged. |
|
@gopherbot backport please 1.13 |
|
The readIdleTimeout is only reset if the client receives packets.
So in the case you mentioned, even if the clients sends requests
frequently, because it doesn't receive any packets, the healthCheck will
still be fired and detects the broken connection.
…On Mon, Mar 15, 2021 at 10:32 PM Kevin Wang ***@***.***> wrote:
Many thanks for this! I've got one question though:
If the application sends http2 requests more frequently than
ReadIdleTimeout (e.g. ReadIdleTimeout=30s, application sends request
every 10s), is it possible that, the application still gets "use of closed
network connection" every 10s, while cc.healthCheck is never triggered
(because it is never idle long enough)?
f, err := cc.fr.ReadFrame()+ if t != nil {+ t.Reset(readIdleTimeout)+ }
if err != nil {
cc.vlogf("http2: Transport readFrame error on conn %p: (%T) %v", cc, err, err)
}
Maybe we should not Reset t? Or only reset t if there it is verified that
there is no error?
I'm probably wrong, please correct me if I'm wrong, thanks! @caesarxuchao
<https://github.com/caesarxuchao>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#55 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVRK2I6NUVIN6XQOIVFBD3TD3UQVANCNFSM4I4CMFIA>
.
--
Regards,
Chao Xu
|
After the connection has been idle for a while, periodic pings are sent over the connection to check its health. Unhealthy connection is closed and removed from the connection pool. Fixes golang/go#31643 Change-Id: Idbbc9cb2d3e26c39f84a33e945e482d41cd8583c GitHub-Last-Rev: 36607fe185ce6684e9900403f82a298ad5567650 GitHub-Pull-Request: golang/net#55 Reviewed-on: https://go-review.googlesource.com/c/net/+/198040 Run-TryBot: Andrew Bonventre <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Russ Cox <[email protected]>
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.
Fixes golang/go#31643