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: Fix bug where items are returned to a sync.Pool incorrectly #4518

Merged
merged 2 commits into from
Oct 21, 2021

Conversation

slim-bean
Copy link
Collaborator

@slim-bean slim-bean commented Oct 21, 2021

#4160 correctly fixed an issue to make sure all iterators are properly closed when nonOverlappingSampleIterator's Close() method is called.

However, there was an unexpected side effect with this change.

It introduced occurences of the iterators close method being called more than once.

Loki has a mix of iterators that will error if closed more than once and others that need to be idempotent, there are some situations where an iterator wrapping another iterator can't always know if the underlying iterator(s) have been closed already so it does result in Close() being called twice.

We discovered a case where the SampleIteratorWithClose was not properly idempotent resulting in a somewhat obscure and difficult to find problem as Cyril described in #4512

The memchunk.go and unordered.go's use of the close function both return elements to a sync.Pool

Our understanding here is that the first call would return the items to the pool correctly, however when any subsequent calls to the iterators close were made, the iterator still holds references to the same items already returned to the pool. These same items could possibly have been lended out by the pool again between calls to the close funtion. This results in items actively in use being put back into the pool where they could be lended out again. Making it possible for multiple separate iterators to all have references to the same items and memory from the pool.

a sync.Once{} has been added to the iterator to make sure the close function is idempotent.

Fixes #4512

@slim-bean slim-bean requested a review from a team as a code owner October 21, 2021 12:27
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Can't comment too much on the code but I think we should add a test for the scenario that we're trying to fix.

@slim-bean
Copy link
Collaborator Author

yeah i pushed this a bit early, there is a better way to apply the sync.Once which is testable

closeFn func() error
closeOnce sync.Once
closeFn func() error
errs []error
Copy link
Contributor

Choose a reason for hiding this comment

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

you could keep the multi error as an error that would simplify the code.

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

@slim-bean slim-bean merged commit d8e6deb into main Oct 21, 2021
@slim-bean slim-bean deleted the return-only-once branch October 21, 2021 15:39
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.

Memory corruption on double close iterator.
3 participants