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.

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

Merged
merged 19 commits into from
Aug 16, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 30 additions & 30 deletions internal/transport/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -428,16 +428,8 @@ func setUpServerOnly(t *testing.T, port int, sc *ServerConfig, ht hType) *server
return server
}

// isGreetingDone verifies that client-server setup is complete
// for the test.
var isGreetingDone = atomic.Bool{}

func setUp(t *testing.T, port int, ht hType, options ...ConnectOptions) (*server, *http2Client, func()) {
var copts = ConnectOptions{}
if len(options) > 0 {
copts = options[0]
}
return setUpWithOptions(t, port, &ServerConfig{}, ht, copts)
func setUp(t *testing.T, port int, ht hType) (*server, *http2Client, func()) {
return setUpWithOptions(t, port, &ServerConfig{}, ht, ConnectOptions{})
}

func setUpWithOptions(t *testing.T, port int, sc *ServerConfig, ht hType, copts ConnectOptions) (*server, *http2Client, func()) {
Expand All @@ -451,7 +443,6 @@ func setUpWithOptions(t *testing.T, port int, sc *ServerConfig, ht hType, copts
cancel() // Do not cancel in success path.
t.Fatalf("failed to create transport: %v", connErr)
}
isGreetingDone.Store(true)
return server, ct.(*http2Client), cancel
}

Expand Down Expand Up @@ -2758,21 +2749,17 @@ func (s) TestClientSendsAGoAwayFrame(t *testing.T) {
}

// hangingConn is a net.Conn wrapper for testing, simulating hanging connections
// after a GOAWAY frame is sent, of which Write operations pause until explicitly signaled
// or a timeout occurs.
// after a GOAWAY frame is sent, of which Write operations pause until explicitly
// signaled or a timeout occurs.
type hangingConn struct {
net.Conn
hangConn chan struct{}
}

func (hc *hangingConn) Read(b []byte) (n int, err error) {
n, err = hc.Conn.Read(b)
return n, err
hangConn chan struct{}
isGreetingDone *atomic.Bool
dfawley marked this conversation as resolved.
Show resolved Hide resolved
}

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
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

@aranjans aranjans Aug 16, 2024

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?

Copy link
Contributor

@purnesh42H purnesh42H Aug 16, 2024

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?

Copy link
Contributor Author

@aranjans aranjans Aug 16, 2024

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.

Copy link
Contributor

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?

timer := time.NewTimer(time.Millisecond * 5)
defer timer.Stop()
Expand All @@ -2784,14 +2771,6 @@ func (hc *hangingConn) Write(b []byte) (n int, err error) {
return n, err
}

func hangingDialer(_ context.Context, addr string) (net.Conn, error) {
conn, err := net.Dial("tcp", addr)
if err != nil {
return nil, err
}
return &hangingConn{Conn: conn, hangConn: make(chan struct{})}, nil
}

// Tests the scenario where a client transport is closed and writing of the
// GOAWAY frame as part of the close does not complete because of a network
// hang. The test verifies that the client transport is closed without waiting
Expand All @@ -2806,9 +2785,30 @@ func (s) TestClientCloseReturnsEarlyWhenGoAwayWriteHangs(t *testing.T) {
goAwayLoopyWriterTimeout = origGoAwayLoopyTimeout
}()

isGreetingDone.Store(false)
// Create the server set up.
connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
dfawley marked this conversation as resolved.
Show resolved Hide resolved
server := setUpServerOnly(t, 0, &ServerConfig{}, normal)
addr := resolver.Address{Addr: "localhost:" + server.port}
isGreetingDone := &atomic.Bool{}
dialer := func(_ context.Context, addr string) (net.Conn, error) {
conn, err := net.Dial("tcp", addr)
if err != nil {
return nil, err
}
isGreetingDone.Store(false)
dfawley marked this conversation as resolved.
Show resolved Hide resolved
return &hangingConn{Conn: conn, hangConn: make(chan struct{}), isGreetingDone: isGreetingDone}, nil
}
copts := ConnectOptions{Dialer: dialer}
copts.ChannelzParent = channelzSubChannel(t)

// Create client transport with custom dialer
ct, connErr := NewClientTransport(connectCtx, context.Background(), addr, copts, func(GoAwayReason) {})
if connErr != nil {
cancel() // Do not cancel in success path.
dfawley marked this conversation as resolved.
Show resolved Hide resolved
t.Fatalf("failed to create transport: %v", connErr)
}
isGreetingDone.Store(true)

server, ct, cancel := setUp(t, 0, normal, ConnectOptions{Dialer: hangingDialer})
defer cancel()
defer server.stop()
dfawley marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
Loading