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

Fix ticker memory leak in bulk indexer due to internal flush call resetting the ticker #797

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

tblyler
Copy link
Contributor

@tblyler tblyler commented Feb 5, 2024

Hello, I discovered that the time.Ticker instance within the BulkIndexer is consuming a bunch of memory in my long running application that creates and closes many instances of the BulkIndexer. See the following screenshots of a pprof heap visualization that I took of the application (forcing a garbage collection run too).

pprof

pprof flamegraph

It appears that the time.Ticker instance is Reset within the flush method, thus causing the memory leak. The fix that I have tested and confirmed working is to simply call Stop on the ticker after the flush call since it is valid to have that Reset call within the flush method for other parts of the BulkIndexer.

I would attach another screenshot of pprof, but the time.Ticker doesn't show up in the visualization due to its small memory footprint.

@Anaethelion Anaethelion self-requested a review February 7, 2024 16:06
Copy link
Contributor

@Anaethelion Anaethelion left a comment

Choose a reason for hiding this comment

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

I agree with your findings and conclusion, especially since we are in a defer.
Nice find and thank you for the patch!

@Anaethelion Anaethelion merged commit dde3be0 into elastic:main Feb 7, 2024
9 checks passed
Anaethelion pushed a commit that referenced this pull request Feb 7, 2024
Anaethelion added a commit that referenced this pull request Feb 7, 2024
…etting the ticker. (#797) (#798)

Co-authored-by: Tony Blyler <tblyler@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants