idle: decrement active call count for streaming RPCs only when the call completes #6610
Conversation
easwars
changed the title
internal/idle/idle_e2e_test.go
Outdated
| // Tests the case where channel idleness is enabled by passing a small value for | ||
| // idle_timeout. Verifies that a READY channel with an ongoing streaming RPC | ||
| // stays READY. | ||
| func (s) TestChannelIdleness_Enabled_OngoingCall_Streaming(t *testing.T) { |
There was a problem hiding this comment.
Can this be merged with the other test? The difference is:
- The server needs to implement something for the streaming handler. It can be done the same way as unary did, though: block on a channel and return nil.
- Instead of just calling the RPC method, the client needs to create the stream and then call Recv(). This could be a little
func()in a testCases struct.
There was a problem hiding this comment.
Rewrote this to be a single test with two subtests for unary and streaming cases.
the client needs to create the stream and then call Recv()
Why is calling Recv() required?
internal/idle/idle_e2e_test.go
Outdated
| // Verify that there are no idleness related channelz events. | ||
| if err := channelzTraceEventNotFound(ctx, "entering idle mode"); err != nil { | ||
| errCh <- err | ||
| return | ||
| } | ||
| if err := channelzTraceEventNotFound(ctx, "exiting idle mode"); err != nil { | ||
| errCh <- err | ||
| return | ||
| } |
There was a problem hiding this comment.
I don't feel super great about these checks, by the way, because if we decided to change the string we log, then the test case is no longer valid but continues to pass. And someone reworking some log strings isn't necessarily going to assume there are test cases relying upon them.
Unless there's corresponding tests that use the same string (ideally it would be a const then) that confirm it went into idle mode by looking for the trace events?
Not something necessarily for this PR but something to think about.
There was a problem hiding this comment.
Fair point. I added a TODO. Will do some thinking to see how we can improve this.
| if cc.WaitForStateChange(sCtx, connectivity.Ready) { | ||
| errCh <- fmt.Errorf("state changed from %q to %q when no state change was expected", connectivity.Ready, cc.GetState()) | ||
| return | ||
| } |
There was a problem hiding this comment.
Couldn't you simplify this test a good amount, remove the errCh, and go back to the old testutils.AwaitNoStateChange, by performing the RPC in a goroutine instead of doing this block in a goroutine?
There was a problem hiding this comment.
If we move the RPC to a goroutine and perform the rest of the checks here, there seems to be a race between how the RPC terminates:
- the
blockChgetting closes, and thereby a normal termination - the context getting cancelled
So, I was having to do way too many checks in different places. The way it currently exists seems easier for error handling.
internal/idle/idle_e2e_test.go
Outdated
| @@ -259,15 +264,14 @@ func (s) TestChannelIdleness_Enabled_OngoingCall(t *testing.T) { | |||
| } | |||
|
|
|||
| // Verify that there are no idleness related channelz events. | |||
| // | |||
| // TODO: Improve the checks here. If these log strings are | |||
| // changed in the code , these checks will continue to pass. | |||
There was a problem hiding this comment.
Remove the space before the comma please. (Never know what other linters will think later unfortunately.)
RELEASE NOTES: