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(common.socket): Switch to context to simplify closing #15589

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Jul 2, 2024

Summary

Switching the socket-close implementation to context cancellation avoids the complexity to keep track of all open connection. As side effects, this PR avoids the memory leak described in #15509 and avoids race conditions when closing the socket while new connections arrive.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15509

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Jul 2, 2024
@srebhan srebhan self-assigned this Jul 3, 2024
@srebhan srebhan mentioned this pull request Jul 3, 2024
@srebhan srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jul 12, 2024
@srebhan srebhan assigned powersj and DStrand1 and unassigned srebhan Jul 12, 2024
@DStrand1 DStrand1 removed their assignment Jul 12, 2024
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Ran my reproducer for the issue with this branch for over 40k metrics and confirmed that the new pprof captures look better than before.

One inline comment about checking an error, otherwise I would like to land and get this in for our bug fix release next Monday.

Thanks!

plugins/common/socket/stream.go Outdated Show resolved Hide resolved
@powersj powersj merged commit 3d9562b into influxdata:master Jul 17, 2024
27 checks passed
@github-actions github-actions bot added this to the v1.31.2 milestone Jul 17, 2024
powersj pushed a commit that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins 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.

memory leak
3 participants