-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Enabling client process multiple GoAways #1393
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -649,47 +649,63 @@ func (t *http2Client) Close() (err error) { | |
return | ||
} | ||
|
||
func (t *http2Client) GracefulClose() error { | ||
t.mu.Lock() | ||
// goAwayActiveStreams is called when a GoAway frame is received. It assumes a transport lock is held. | ||
func (t *http2Client) goAwayActiveStreams() { | ||
// A client can recieve multiple GoAways from server (look at https://github.com/grpc/grpc-go/issues/1387). | ||
// The idea is that the first GoAway will be sent with an ID of MaxInt32 and the second GoAway will be sent after an RTT delay | ||
// with the ID of the last stream the server will process. | ||
// Therefore, when we get the first GoAway we don't really close any streams. While in case of second GoAway we | ||
// close all streams created after the second GoAwayId. This way streams that were in-flight while the GoAway from server | ||
// was being sent don't get killed. | ||
// | ||
// Note: to be backward compatible with servers that will still send only 1 GoAway we'll still try and close streams with ID | ||
// greater that GoAwayId sent by the first GoAway. | ||
|
||
n := t.prevGoAwayID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you rename |
||
if n == 0 && t.nextID > 1 { | ||
n = t.nextID - 2 | ||
} | ||
m := t.goAwayID + 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pass goAwayID as a parameter (lastValidStreamID or something)? Then you can delete the field in http2client IIUC. |
||
if m == 2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really a special case of a general problem. If a server says GOAWAY(4), which is valid, we need to kill streams 5->max. This code does not handle that. How about: // Round to a multiple of two, then add one.
start := ((lastValidStreamID + 1) % 2) + 1 |
||
m = 1 | ||
} | ||
for i := m; i <= n; i += 2 { | ||
if s, ok := t.activeStreams[i]; ok { | ||
close(s.goAway) | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. t.prevGoAwayID probably should be set to
the second goaway will go through 1B iterations. OR, we can do something like this, above: if t.nextID == 1 { // or len(t.activeStreams) == 0
return // there are no streams to stop
}
end := t.prevGoAwayID
if end == 0 || end > t.nextID {
end = t.nextID - 2
} |
||
|
||
// gracefulCloseLocked assumes that transport lock is held. | ||
// It will return true if there are no active streams left or | ||
// the tranport is in unreachable state, in such a case the transport should be closed | ||
func (t *http2Client) gracefulCloseLocked() bool { | ||
switch t.state { | ||
case unreachable: | ||
// The server may close the connection concurrently. t is not available for | ||
// any streams. Close it now. | ||
t.mu.Unlock() | ||
t.Close() | ||
return nil | ||
return true | ||
case closing: | ||
t.mu.Unlock() | ||
return nil | ||
} | ||
// Notify the streams which were initiated after the server sent GOAWAY. | ||
select { | ||
case <-t.goAway: | ||
n := t.prevGoAwayID | ||
if n == 0 && t.nextID > 1 { | ||
n = t.nextID - 2 | ||
} | ||
m := t.goAwayID + 2 | ||
if m == 2 { | ||
m = 1 | ||
} | ||
for i := m; i <= n; i += 2 { | ||
if s, ok := t.activeStreams[i]; ok { | ||
close(s.goAway) | ||
} | ||
} | ||
default: | ||
return false | ||
} | ||
if t.state == draining { | ||
t.mu.Unlock() | ||
return nil | ||
return false | ||
} | ||
t.state = draining | ||
active := len(t.activeStreams) | ||
t.mu.Unlock() | ||
if active == 0 { | ||
return true | ||
} | ||
return false | ||
} | ||
|
||
func (t *http2Client) GracefulClose() error { | ||
t.mu.Lock() | ||
if t.gracefulCloseLocked() { | ||
t.mu.Unlock() | ||
return t.Close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about doing the same thing with t.Close() ? t.closeLocked() which assumes t.mu is already taken. If this ends up causing a bunch of work or ugliness, then don't. But there are other places that follow the unlock->Close() pattern. It's not a performance problem, since this is not a critical path, but I worry a bit about races if you give up the lock after you determine you should shut down but before doing so. |
||
} | ||
t.mu.Unlock() | ||
return nil | ||
} | ||
|
||
|
@@ -1008,13 +1024,25 @@ func (t *http2Client) handleGoAway(f *http2.GoAwayFrame) { | |
} | ||
t.prevGoAwayID = id | ||
t.goAwayID = f.LastStreamID | ||
t.goAwayActiveStreams() | ||
shouldCloseT := t.gracefulCloseLocked() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you do this in t.goAwayActiveStreams() to avoid the duplication below? |
||
t.mu.Unlock() | ||
if shouldCloseT { | ||
t.Close() | ||
} | ||
return | ||
default: | ||
t.setGoAwayReason(f) | ||
} | ||
t.goAwayID = f.LastStreamID | ||
close(t.goAway) | ||
t.goAwayActiveStreams() | ||
shouldCloseT := t.gracefulCloseLocked() | ||
t.mu.Unlock() | ||
if shouldCloseT { | ||
t.Close() | ||
} | ||
return | ||
} | ||
t.mu.Unlock() | ||
} | ||
|
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.
I would omit this comment, personally, or at least the backward compatible bit.. The spec requires this behavior.