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.

Sign up for GitHub

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

Jump to bottom

client: include details about GOAWAYs in status messages #4316

Merged
merged 11 commits into from Apr 24, 2021
Merged

client: include details about GOAWAYs in status messages #4316

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ba8e17b
Propagate errors causing connection close to RPC statuses
apolcyn Mar 31, 2021
0640203
fix transport tests
apolcyn Apr 1, 2021
38da1fd
Make test deterministic
apolcyn Apr 1, 2021
b1f5e59
Include details about possible goaways when terminating RPCs due to
apolcyn Apr 1, 2021
7dee6ae
skip handler-tls config for max age goaway error test
apolcyn Apr 2, 2021
052d632
Merge remote-tracking branch 'upstream/master' into propagage_goaway_…
apolcyn Apr 14, 2021
eef6755
make test use stubserver
apolcyn Apr 14, 2021
afed69f
address review comment
apolcyn Apr 14, 2021
9277ca5
address comment from https://github.com/grpc/grpc-go/pull/4311#discus…
apolcyn Apr 14, 2021
5270ce5
fix typo
apolcyn Apr 14, 2021
c23ff7f
Merge remote-tracking branch 'upstream/master' into propagage_goaway_…
apolcyn Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 18 additions & 2 deletions internal/transport/http2_client.go
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
package transport

import (
"bytes"
"context"
"fmt"
"io"
Expand Down Expand Up @@ -116,6 +117,9 @@ type http2Client struct {
// goAwayReason records the http2.ErrCode and debug data received with the
// GoAway frame.
goAwayReason GoAwayReason
// goAwayDebugMessage contains a detailed human readable string about a
// GoAway frame, useful for error messages.
goAwayDebugMessage string
// A condition variable used to signal when the keepalive goroutine should
// go dormant. The condition for dormancy is based on the number of active
// streams and the `PermitWithoutStream` keepalive client parameter. And
Expand Down Expand Up @@ -874,6 +878,12 @@ func (t *http2Client) Close(err error) error {
if channelz.IsOn() {
channelz.RemoveEntry(t.channelzID)
}
// Append info about previous goaways if there were any, since this may be important
// for understanding the root cause for this connection to be closed.
_, goAwayDebugMessage := t.GetGoAwayReason()
if len(goAwayDebugMessage) > 0 {
err = fmt.Errorf("closing transport due to: %v, received prior goaway: %v", err, goAwayDebugMessage)
}
// Notify all active streams.
for _, s := range streams {
t.closeStream(s, err, false, http2.ErrCodeNo, status.New(codes.Unavailable, ErrConnClosing.Desc), nil, false)
Expand Down Expand Up @@ -1215,12 +1225,18 @@ func (t *http2Client) setGoAwayReason(f *http2.GoAwayFrame) {
t.goAwayReason = GoAwayTooManyPings
}
}
var m bytes.Buffer
m.WriteString("code: ")
m.WriteString(f.ErrCode.String())
m.WriteString(", debug data: ")
m.Write(f.DebugData())
t.goAwayDebugMessage = m.String()
Copy link
Member

Choose a reason for hiding this comment

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

Is this better than t.goAwayDebugMessage = fmt.Sprintf("code: %s, debug data: %v", f.ErrCode, f.DebugData) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks better, changed to that

}

func (t *http2Client) GetGoAwayReason() GoAwayReason {
func (t *http2Client) GetGoAwayReason() (GoAwayReason, string) {
t.mu.Lock()
defer t.mu.Unlock()
return t.goAwayReason
return t.goAwayReason, t.goAwayDebugMessage
}

func (t *http2Client) handleWindowUpdate(f *http2.WindowUpdateFrame) {
Expand Down
42 changes: 21 additions & 21 deletions internal/transport/keepalive_test.go
View file
Open in desktop
Expand Up @@ -47,7 +47,7 @@ func (s) TestMaxConnectionIdle(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand All @@ -68,7 +68,7 @@ func (s) TestMaxConnectionIdle(t *testing.T) {
if !timeout.Stop() {
<-timeout.C
}
if reason := client.GetGoAwayReason(); reason != GoAwayNoReason {
if reason, _ := client.GetGoAwayReason(); reason != GoAwayNoReason {
t.Fatalf("GoAwayReason is %v, want %v", reason, GoAwayNoReason)
}
case <-timeout.C:
Expand All @@ -86,7 +86,7 @@ func (s) TestMaxConnectionIdleBusyClient(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -122,7 +122,7 @@ func (s) TestMaxConnectionAge(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand All @@ -142,7 +142,7 @@ func (s) TestMaxConnectionAge(t *testing.T) {
if !timeout.Stop() {
<-timeout.C
}
if reason := client.GetGoAwayReason(); reason != GoAwayNoReason {
if reason, _ := client.GetGoAwayReason(); reason != GoAwayNoReason {
t.Fatalf("GoAwayReason is %v, want %v", reason, GoAwayNoReason)
}
case <-timeout.C:
Expand All @@ -169,7 +169,7 @@ func (s) TestKeepaliveServerClosesUnresponsiveClient(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -228,7 +228,7 @@ func (s) TestKeepaliveServerWithResponsiveClient(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -257,7 +257,7 @@ func (s) TestKeepaliveClientClosesUnresponsiveServer(t *testing.T) {
PermitWithoutStream: true,
}}, connCh)
defer cancel()
defer client.Close()
defer client.Close(nil)

conn, ok := <-connCh
if !ok {
Expand Down Expand Up @@ -288,7 +288,7 @@ func (s) TestKeepaliveClientOpenWithUnresponsiveServer(t *testing.T) {
Timeout: 1 * time.Second,
}}, connCh)
defer cancel()
defer client.Close()
defer client.Close(nil)

conn, ok := <-connCh
if !ok {
Expand Down Expand Up @@ -317,7 +317,7 @@ func (s) TestKeepaliveClientClosesWithActiveStreams(t *testing.T) {
Timeout: 1 * time.Second,
}}, connCh)
defer cancel()
defer client.Close()
defer client.Close(nil)

conn, ok := <-connCh
if !ok {
Expand Down Expand Up @@ -352,7 +352,7 @@ func (s) TestKeepaliveClientStaysHealthyWithResponsiveServer(t *testing.T) {
PermitWithoutStream: true,
}})
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -391,7 +391,7 @@ func (s) TestKeepaliveClientFrequency(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, normal, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand All @@ -402,7 +402,7 @@ func (s) TestKeepaliveClientFrequency(t *testing.T) {
if !timeout.Stop() {
<-timeout.C
}
if reason := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
if reason, _ := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
t.Fatalf("GoAwayReason is %v, want %v", reason, GoAwayTooManyPings)
}
case <-timeout.C:
Expand Down Expand Up @@ -436,7 +436,7 @@ func (s) TestKeepaliveServerEnforcementWithAbusiveClientNoRPC(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, normal, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand All @@ -447,7 +447,7 @@ func (s) TestKeepaliveServerEnforcementWithAbusiveClientNoRPC(t *testing.T) {
if !timeout.Stop() {
<-timeout.C
}
if reason := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
if reason, _ := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
t.Fatalf("GoAwayReason is %v, want %v", reason, GoAwayTooManyPings)
}
case <-timeout.C:
Expand Down Expand Up @@ -480,7 +480,7 @@ func (s) TestKeepaliveServerEnforcementWithAbusiveClientWithRPC(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand All @@ -497,7 +497,7 @@ func (s) TestKeepaliveServerEnforcementWithAbusiveClientWithRPC(t *testing.T) {
if !timeout.Stop() {
<-timeout.C
}
if reason := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
if reason, _ := client.GetGoAwayReason(); reason != GoAwayTooManyPings {
t.Fatalf("GoAwayReason is %v, want %v", reason, GoAwayTooManyPings)
}
case <-timeout.C:
Expand Down Expand Up @@ -530,7 +530,7 @@ func (s) TestKeepaliveServerEnforcementWithObeyingClientNoRPC(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, normal, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -564,7 +564,7 @@ func (s) TestKeepaliveServerEnforcementWithObeyingClientWithRPC(t *testing.T) {
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -604,7 +604,7 @@ func (s) TestKeepaliveServerEnforcementWithDormantKeepaliveOnClient(t *testing.T
}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, normal, clientOptions)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down Expand Up @@ -658,7 +658,7 @@ func (s) TestTCPUserTimeout(t *testing.T) {
},
)
defer func() {
client.Close()
client.Close(nil)
server.stop()
cancel()
}()
Expand Down
5 changes: 3 additions & 2 deletions internal/transport/transport.go
View file
Open in desktop
Expand Up @@ -656,8 +656,9 @@ type ClientTransport interface {
// HTTP/2).
GoAway() <-chan struct{}

// GetGoAwayReason returns the reason why GoAway frame was received.
GetGoAwayReason() GoAwayReason
// GetGoAwayReason returns the reason why GoAway frame was received, along
// with a human readable string with debug info.
GetGoAwayReason() (GoAwayReason, string)

// RemoteAddr returns the remote network address.
RemoteAddr() net.Addr
Expand Down