test: fix flaky test ClientSendsAGoAway #7224
Conversation
aranjans
force-pushed
the
aranjans_7160
branch
2 times, most recently
from
3753ade to
63ba062
Compare
|
Probably need some more description about the fix |
test/goaway_test.go
Outdated
| @@ -816,6 +818,7 @@ func (s) TestClientSendsAGoAway(t *testing.T) { | |||
| } | |||
| } | |||
| }() | |||
| cc.WaitForStateChange(ctx, connectivity.Connecting) | |||
There was a problem hiding this comment.
what would be the max wait here? Timeout for all tests is 7m. Is there a chance it can breach that?
There was a problem hiding this comment.
Max timeout is as long context is not timed out.
There was a problem hiding this comment.
shouldn't the state to wait be connectivity.Ready?
There was a problem hiding this comment.
I think it depends where the race is happening.
If there's just a race where we call Close before we even start creating the connection, then this is a problem with the test, and Connecting should fix that.
If this is a race where a connection is established and doesn't get a GOAWAY written to it (due to internal state in the ClientConn/addrConn, then Connecting will expose that race and we should fix it.
There was a problem hiding this comment.
@purnesh42H WaitForStateChange waits until the connectivity.State of ClientConn changes from sourceState or ctx expires. A true value is returned in former case and false in latter. So we are providing sourceState as Connecting and hence it will wait until state changes.
There was a problem hiding this comment.
I think race is detected when client conn is getting closed before we try to send goAway so believe just waiting for clientConn state change will fix this race condition.
test/goaway_test.go
Outdated
| @@ -800,6 +800,7 @@ func (s) TestClientSendsAGoAway(t *testing.T) { | |||
| for { | |||
| f, err := ct.fr.ReadFrame() | |||
| if err != nil { | |||
| t.Logf("error reading frame: %v", err) | |||
test/goaway_test.go
Outdated
| @@ -816,6 +818,7 @@ func (s) TestClientSendsAGoAway(t *testing.T) { | |||
| } | |||
| } | |||
| }() | |||
| cc.WaitForStateChange(ctx, connectivity.Connecting) | |||
There was a problem hiding this comment.
I think it depends where the race is happening.
If there's just a race where we call Close before we even start creating the connection, then this is a problem with the test, and Connecting should fix that.
If this is a race where a connection is established and doesn't get a GOAWAY written to it (due to internal state in the ClientConn/addrConn, then Connecting will expose that race and we should fix it.
|
From chat offline:
So I think the test is correct (though it can still be simplified), but there is a race in the code that prevents the GOAWAY from being sent in some cases. |
|
@dfawley updated the PR. |
|
Update: this is the code where we are hardcode closing the transport. Will update the PR with the change where we send goAway frame in this case as well. |
|
@dfawley As per our discussion offline, we won't be waiting for all the transport conn to close down. So this PR's change is ready to review as it just makes sure we are waiting for the clientconn to be READY. |
dfawley
changed the title
Fixes #7160
fix:
goaway_test/TestClientSendsAGoAwaydidn't use to wait for channels to be ready and which is why sometimes it was not able to read the frame and eventually result in test failure. This PR change make sure we read the frame only when the channel's state becomes READY.RELEASE NOTES: n/a