Skip to content

fix: log and trace processor memory leak#4449

Merged
lzchen merged 9 commits intoopen-telemetry:mainfrom
jomcgi:fix-batchlogprocessor-gc
Apr 9, 2025
Merged

fix: log and trace processor memory leak#4449
lzchen merged 9 commits intoopen-telemetry:mainfrom
jomcgi:fix-batchlogprocessor-gc

Conversation

@jomcgi
Copy link
Contributor

@jomcgi jomcgi commented Feb 26, 2025

Description

Strong references in register_at_fork persist after the processor objects are deleted.
This prevents garbage collection as the reference count for the processor object never drops to 0.
Update register_at_fork calls in processors to use weak references
Add tests for all processors that us register_at_fork

Fixes #4422

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • tox -e opentelemetry-sdk

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jomcgi jomcgi changed the title fix: log and trace processor memory leak [WIP] fix: log and trace processor memory leak Feb 26, 2025
@jomcgi jomcgi force-pushed the fix-batchlogprocessor-gc branch 5 times, most recently from 95a4774 to da51419 Compare February 26, 2025 23:01
Update register_at_fork calls in processors to use weak references
Add tests for all processors that us register_at_fork
Strong references in register_at_fork persist after the processor
objects are deleted.
This prevents garbage collection as the reference count for the
processor object never drops to 0.
@jomcgi jomcgi force-pushed the fix-batchlogprocessor-gc branch from da51419 to 8f0efad Compare February 26, 2025 23:14
@jomcgi jomcgi changed the title [WIP] fix: log and trace processor memory leak fix: log and trace processor memory leak Feb 26, 2025
@jomcgi jomcgi marked this pull request as ready for review February 26, 2025 23:24
@jomcgi jomcgi requested a review from a team as a code owner February 26, 2025 23:24
@xrmx xrmx moved this to Ready for review in Python PR digest Feb 28, 2025
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks LGTM! Since there is no way to unregister the actual at fork hook, this still leaks those lambdas but I imagine it's negligible.

Copy link
Contributor

@lzchen lzchen 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!

@lzchen lzchen merged commit 4dc6b3b into open-telemetry:main Apr 9, 2025
386 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in Python PR digest Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

BatchLogRecordProcessor objects are not able to be garbage collected

5 participants