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 caused by throwableToSpan #2227

Merged
merged 2 commits into from Aug 30, 2022
Merged

Conversation

adinauer
Copy link
Member

📜 Description

The Hub holds on to a WeakHashMap called throwableToSpan which maps root causes of exceptions to Spans. The memory leak was caused by the Span holding a strong reference to the throwable which contains the root cause that is also used as the key for the WeakHashMap. This means the value of the WeakHashMap was causing its own key not to be garbage collected and the memory was never freed.

The fix is to weakly hold on to the Span so it can be garbage collected and thus also the key can be garbage collected afterwards.

💡 Motivation and Context

Fixes #2225

💚 How did you test it?

Manual testing using debugger on a modified sample application, then using visualvm to trigger GC manually.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@adinauer adinauer requested a review from romtsn as a code owner August 30, 2022 11:06
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thanks Alex

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Base: 80.64% // Head: 80.62% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (b372548) compared to base (d425298).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2227      +/-   ##
============================================
- Coverage     80.64%   80.62%   -0.02%     
  Complexity     3357     3357              
============================================
  Files           240      240              
  Lines         12331    12335       +4     
  Branches       1636     1638       +2     
============================================
+ Hits           9944     9945       +1     
- Misses         1781     1783       +2     
- Partials        606      607       +1     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/Hub.java 74.75% <50.00%> (-0.51%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adinauer adinauer merged commit 88fade9 into main Aug 30, 2022
@adinauer adinauer deleted the fix/oom-throwable-to-span branch August 30, 2022 11:56
vaind pushed a commit that referenced this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory Leak - Object throwableToSpan keeps growing over time
3 participants