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

Loki: Bug: frontend waiting on results which would never come #4411

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

slim-bean
Copy link
Collaborator

We were not also checking the context to see if it was canceled while waiting for goroutines to return with query results leading to a leak of goroutines.

…r query results leading to goroutines waiting forever for results that would never come...
@slim-bean slim-bean requested a review from a team as a code owner October 5, 2021 12:47
if resp.err != nil {
return nil, resp.err
select {
case <-ctx.Done():
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Should we put some info or error log (recording ctx.Error()) when we know we stop because of ctx.Done()?

Wouldn't it help to understand why we got partial results in those cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Since he now returns the error it's the caller that can log and do whatever he wants with the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cyriltovena Agree :) I added that comment before the return statement was added.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM!

@slim-bean slim-bean merged commit d5ec714 into main Oct 5, 2021
@slim-bean slim-bean deleted the fix-frontend-canceled-calls branch October 5, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants