Fix memory leak in NodeTaskMap#15856
Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom Mar 22, 2021
Merged
Conversation
mbasmanova
approved these changes
Mar 22, 2021
Contributor
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@mayankgarg1990 Thanks for tracking down and fixing this issue. CC: @bhhari
Contributor
There was a problem hiding this comment.
perhaps, add a comment explaining that this cannot be broken down into 3 separate finalizers as this is non-obvious
Author
There was a problem hiding this comment.
I have added the necessary comment
This fixes a leak introduced recently in 26a373c. `memoryUsageTracker` and `cpuUtilizationPercentageTracker` were being passed as both the referrant and as a part of the cleanup function. This caused 2 references to these objects - one from the `FinalizerService#finalizers` and `FinalizerService#FinalizeReference#cleanup` and prevented a cleanup. Fixing this to just pass all 3 cleanups in a single function along with the `NodeStatsTracker` object.
mayankgarg1990
force-pushed
the
fix_memory_leak
branch
from
ffce17a to
2673c25
Compare
Contributor
|
Great catch. Thank you @mayankgarg1990 |
tdcmeehan
approved these changes
Mar 22, 2021
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a leak introduced recently in 26a373c.
memoryUsageTrackerandcpuUtilizationPercentageTrackerwere being passed as both the referrant and as a part of the cleanup function. This caused 2 references to
these objects - one from the
FinalizerService#finalizersandFinalizerService#FinalizeReference#cleanupand prevented a cleanup.
Fixing this to just pass all 3 cleanups in a single function along with the
NodeStatsTrackerobject.Test plan - Using a heapdump I could now ensure that entries were indeed being cleaned up