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

context/data: memleak due to API (mis)use #131

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bradh352
Copy link
Contributor

@bradh352 bradh352 commented Feb 6, 2025

Memory management is a foreign concept to python in general, however it
appears DNode and Context require .free() and .destroy() to be called,
respectively, to avoid a memory leak.

This seems easily avoidable by simply attaching a destructor to the
class so the python garbage collector can clean up automatically when all
references go out of scope.

We add a reference to any DNode object derived from another DNode object
(but not duplicated) so we can determine if we are allowed to be the one
to destroy the DNode cdata, as well as to keep proper reference counting
in case the owner goes out of scope but a dependent reference is still
in scope.

This change should also be compatible with existing integrations which may be
aware of this as it sets the internal cdata reference to None and checks
it to ensure it doesn't run the cleanup again. That said, I think calling
DNode.free() in this case is still risky since there are no checks to
ensure this is truly valid like the destructor does.

Signed-off-by: Brad House (@bradh352)

@bradh352
Copy link
Contributor Author

bradh352 commented Feb 6, 2025

guess I need to dig into the test system to see what it doesn't like about this....

Memory management is a foreign concept to python in general, however it
appears DNode and Context require .free() and .destroy() to be called,
respectively, to avoid a memory leak.

This seems easily avoidable by simply attaching a destructor to the
class so the python garbage collector can clean up automatically when all
references go out of scope.

We add a reference to any DNode object derived from another DNode object
(but not duplicated) so we can determine if we are allowed to be the one
to destroy the DNode cdata, as well as to keep proper reference counting
in case the owner goes out of scope but a dependent reference is still
in scope.

This change should also be compatible with existing integrations which may be
aware of this as it sets the internal `cdata` reference to None and checks
it to ensure it doesn't run the cleanup again.  That said, I think calling
DNode.free() in this case is still risky since there are no checks to
ensure this is truly valid like the destructor does.

Signed-off-by: Brad House <[email protected]>
@bradh352 bradh352 marked this pull request as draft February 7, 2025 14:04
@bradh352
Copy link
Contributor Author

bradh352 commented Feb 7, 2025

tests are getting further, but it looks like I'll need to dig deeper, this is fairly hard to debug since its crashing in C code, not sure how to dereference the python line that causes the crash or if its in the garbage collecting process.

likely i need to extend the reference linking even further to additional child structures from DNode, and possibly also enforce each DNode references the Context it is associated with, it looks like in a few places that may not be true.

marking as a draft, going to put this on hold as I've sprinkled some .free() and .destroy() in the implementation I'm working on for now ...
bradh352/sonic-buildimage@5019c24

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.

1 participant