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: memory leak in influx parser #9787

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Sep 22, 2021

Every time the code reached the end of the previous buffered read data, and had an incomplete metric, it would do a read to re-fill the buffer, and then because the buffer was now full, would mistakenly double the buffer's size.

It's obvious from the code that the intent was to grow the buffer if it did not contain a complete metric and the buffer was full. Rearranging the code does this, and fixes the problem.

I classified this as a memory leak, as while the memory isn't lost, usage grows uncontrollably and is never reclaimed.

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

resolves #

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 22, 2021
Every time the code reached the end of the previous buffered read data, and had an incomplete metric, it would do a read to re-fill the buffer, and then because the buffer was now full, would mistakenly double the buffer's size.
@phemmer phemmer force-pushed the fix-machine-memleak branch from 8f9451b to 666dd32 Compare September 22, 2021 01:45
@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 22, 2021
@reimda reimda self-requested a review September 22, 2021 16:09
@reimda
Copy link
Contributor

reimda commented Sep 24, 2021

Hi Patrick, thanks for tracking down this memory leak.

There is an open PR to switch to a newer line protocol parser that is shared with other InfluxData projects (#9685). We're not quite ready to merge it because there was a report of a panic a couple days ago.

This leak fix may be useful if the panic or something else prevents us from switching parsers, but I wanted to make sure you knew the long term plans for the parser.

I'd be interested in your opinion on the new parser if you have time to take a look at it.

Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! lgtm, we are still working on the new line protocol pr (moved to here: #9871) so taking this fix now seems like a good idea.

@sspaink sspaink merged commit 128ed88 into influxdata:master Oct 7, 2021
reimda pushed a commit that referenced this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants