-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
transport: add timeout for writing GOAWAY on http2Client.Close() #7371
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7371 +/- ##
==========================================
+ Coverage 81.58% 81.85% +0.27%
==========================================
Files 358 360 +2
Lines 27388 27539 +151
==========================================
+ Hits 22345 22543 +198
+ Misses 3828 3796 -32
+ Partials 1215 1200 -15
|
8e61c11
to
3ea4a82
Compare
c81d450
to
3684229
Compare
3363377
to
87da73c
Compare
84d59c4
to
205fbdf
Compare
internal/transport/transport_test.go
Outdated
} | ||
|
||
func (hc *hangingConn) Write(b []byte) (n int, err error) { | ||
n, err = hc.Conn.Write(b) | ||
if isGreetingDone.Load() == true { | ||
if hc.isGreetingDone.Load() == true { | ||
// Hang the Write for more than goAwayLoopyWriterTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this timer? Otherwise it will release on its own and pass on master@HEAD, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping, this doesn't seem done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm... I think we would need to have timeout for hc.hangConn
, otherwise this go-routine will be leaked via this call. So hc.Conn.Write
will be hang after we made this write to the conn, which will eventually make the above go-routine to run forever and end up in leaked go-routine.(btw we already have deadline for net.Conn.Write defined here at the top of ct.Close, but then the current test will test only for this loopyWriter to timeout).
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely familiar with the goroutine you are referring to but one suggestion is that you probably don't have to do the actual write n, err = hc.Conn.Write(b)
in case of goaway? You can just compare the passed bytes b
if its goaway and then just hang? or is that not possible because different structs are being written?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, actually we don't even need to compare it, we can just make sure hangingConn hangs from the time it receives the goAway request.
@dfawley I have removed the timer logic for hc.Write
, and updated the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably good to release the <-hc.hangConn
at the end of test?
I'd really like to wrap this up before we cut the release branch. Will that be possible? |
411a244
to
15f6613
Compare
15f6613
to
aebe562
Compare
internal/transport/transport_test.go
Outdated
case <-hc.hangConn: | ||
case <-timer.C: | ||
} | ||
if n == goAwayFrameSize { // hang the conn after the goAway is received |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the atomic?
This is worse, since you're just assuming that: 1. Nothing else written will be the size of the GOAWAY frame, and 2. the GOAWAY will be written to the connection, by itself, in a single operation.
internal/transport/transport_test.go
Outdated
} | ||
|
||
func (hc *hangingConn) Write(b []byte) (n int, err error) { | ||
n, err = hc.Conn.Write(b) | ||
if n == goAwayFrameSize { // hang the conn after the goAway is received | ||
if hc.startHanging.Load() == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if x == true
--> if x
, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
2972676
to
74efc67
Compare
74efc67
to
b52c896
Compare
b52c896
to
fad99ba
Compare
fixes #7314
Problem
In gRPC 1.64, we released a feature "Send GOAWAY to server on Client Transport Shutdown", post which in case of network hang when we are not able to write GOAWAY frame from client, closing connections will take a very long time.
What does this PR fix
This PR fixes this issue by having a timeout for loopyWriter to write GOAWAY so that http2Client.Close() doesn't wait for too long.
RELEASE NOTES: