Address post-merge feedback for #6471 #6505
Conversation
| require.NoError(t, querierLoop.CloseSend()) | ||
| cancel() | ||
| require.NoError(t, util.CloseAndExhaust[*schedulerpb.SchedulerToQuerier](querierLoop)) |
There was a problem hiding this comment.
Note to reviewers: moving the call to cancel() before closing the stream is required because otherwise CloseAndExhaust() will block: the stream has not ended, and the server does not know to close the stream without the cancellation. (Previously, CloseSend() returned immediately, and the call to cancel() was what made the test pass.)
charleskorn
force-pushed
the
charleskorn/post-6471-feedback
branch
from
6dab7c3 to
2635d15
Compare
…e the stream synchronously after encountering an error
| return nil | ||
| go func() { | ||
| for { | ||
| if _, err := stream.Recv(); err != nil { |
There was a problem hiding this comment.
After 3 seconds the function will return but this goroutine will continue to consume in the background. What if the consume blocks forever? We're going to leak goroutines. So question is: do we ensure that the stream context is canceled on the client side (this side) so that the Recv() will unblock and goroutine terminate?
There was a problem hiding this comment.
For almost all cases where we now use CloseAndExhaust, the streaming call is made in the context of an incoming HTTP or gRPC request (eg. querier evaluating a query), and that context will be cancelled when the response is returned. If for some reason that context isn't already being cancelled today, then we're potentially leaking the stream, but then CloseAndExhaust should help mitigate that by exhausting the stream and then terminating.
The only exception to this case is frontendSchedulerWorker which is used by query-frontends to queue requests to query-schedulers. In this case, the context will always be cancelled after CloseAndExhaust returns.
* Fix typo in comment. * Use more efficient `SpanLogger.DebugLog()` method in more places. * Add linting rule to forbid the direct use of CloseSend(). * Add documentation to CloseAndExhaust. * Update changelog entry. * Report correct error when closing loop fails * Avoid reading entire store-gateway query stream response when one or more calls fail. * Silence linting warnings. * Ensure CloseAndExhaust never blocks forever. * Fix race in ingester streaming tests: there's no guarantee we'll close the stream synchronously after encountering an error * Address PR feedback: reduce timeout. (cherry picked from commit 0103407)
* Fix typo in comment. * Use more efficient `SpanLogger.DebugLog()` method in more places. * Add linting rule to forbid the direct use of CloseSend(). * Add documentation to CloseAndExhaust. * Update changelog entry. * Report correct error when closing loop fails * Avoid reading entire store-gateway query stream response when one or more calls fail. * Silence linting warnings. * Ensure CloseAndExhaust never blocks forever. * Fix race in ingester streaming tests: there's no guarantee we'll close the stream synchronously after encountering an error * Address PR feedback: reduce timeout. (cherry picked from commit 0103407) Co-authored-by: Charles Korn <[email protected]>
* Fix typo in comment. * Use more efficient `SpanLogger.DebugLog()` method in more places. * Add linting rule to forbid the direct use of CloseSend(). * Add documentation to CloseAndExhaust. * Update changelog entry. * Report correct error when closing loop fails * Avoid reading entire store-gateway query stream response when one or more calls fail. * Silence linting warnings. * Ensure CloseAndExhaust never blocks forever. * Fix race in ingester streaming tests: there's no guarantee we'll close the stream synchronously after encountering an error * Address PR feedback: reduce timeout.
What this PR does
This PR addresses some feedback from #6471:
level.Debug(spanLogger).Log(...)pattern withspanLogger.DebugLog(...): c3c1437CloseSend()on client streaming gRPC streams: f04bb75CloseAndExhaustnever blocks forever: 2635d15It also fixes an issue where the wrong error value was logged while shutting down the query-scheduler communication loop in query-frontends: df78279
I'd suggest reviewing each commit individually.
Which issue(s) this PR fixes or relates to
#6471
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]