Enabling client process multiple GoAways #1393
Conversation
|
Also, as discussed, please move all the go away handling code into handleGoAway. |
clientconn.go
Outdated
| if drain { | ||
| t.GracefulClose() | ||
| } else { | ||
| if !drain { |
transport/http2_client.go
Outdated
| @@ -665,6 +665,16 @@ func (t *http2Client) GracefulClose() error { | |||
| // Notify the streams which were initiated after the server sent GOAWAY. | |||
| select { | |||
| case <-t.goAway: | |||
| // 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 an ID of the last stream the server will process. | |||
transport/http2_client.go
Outdated
| // 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 |
There was a problem hiding this comment.
I would omit this comment, personally, or at least the backward compatible bit.. The spec requires this behavior.
transport/http2_client.go
Outdated
| if n == 0 && t.nextID > 1 { | ||
| n = t.nextID - 2 | ||
| } | ||
| m := t.goAwayID + 2 |
There was a problem hiding this comment.
pass goAwayID as a parameter (lastValidStreamID or something)? Then you can delete the field in http2client IIUC.
transport/http2_client.go
Outdated
| n = t.nextID - 2 | ||
| } | ||
| m := t.goAwayID + 2 | ||
| if m == 2 { |
There was a problem hiding this comment.
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
transport/http2_client.go
Outdated
| // 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.
Can you rename n -> end or something with meaning, please?
transport/http2_client.go
Outdated
| close(s.goAway) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
t.prevGoAwayID probably should be set to end - 2 here. Otherwise if we get
GOAWAY(INT32_MAX)
GOAWAY(0)
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
}
transport/http2_client.go
Outdated
| func (t *http2Client) GracefulClose() error { | ||
| t.mu.Lock() | ||
| if t.gracefulCloseLocked() { | ||
| t.mu.Unlock() | ||
| return t.Close() |
There was a problem hiding this comment.
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.
transport/http2_client.go
Outdated
| @@ -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.
Can you do this in t.goAwayActiveStreams() to avoid the duplication below?
transport/http2_client.go
Outdated
| if upperLimit == 0 { // This is the first GoAway Frame. | ||
| upperLimit = math.MaxUint32 // Kill all streams after the GoAway ID. | ||
| } | ||
| for k, v := range t.activeStreams { |
There was a problem hiding this comment.
Nit:
for id, s := range t.activeStreams {| t.mu.Unlock() | ||
| if active == 0 { |
There was a problem hiding this comment.
If there are no active streams, can we close and return above the select, even? Or do we care about the GOAWAY even though there are no streams?
There was a problem hiding this comment.
As discussed, don't bother with this.
#1387 The first of two parts.