bugfix: KernelPtr objects are owned by the user by default #10
Conversation
This file contains 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
Before this commit, KernelPtr objects created through its _from_ptr constructor would not be owned by the user. This is incorrect, in most cases the user is responsible for freeing up these pointers. Only in the case of callbacks (e.g. for notifications and logging) would the pointer not be owned by the user. However, this usage is currently not implemented. Fix this by making all KernelPtr objects owned by the user by default, while still allowing this to be overridden in the future (e.g. when notifications are more fully implemented) through the _from_ptr constructor.
ismaelsadeeq
approved these changes
Dec 26, 2024
There was a problem hiding this comment.
Tested ACK 309fe70
Thank you for the swift fix @stickies-v
I've tested this branch locally, the memory leak is gone now.
I processed around 300 blocks for this test and the memory usage is very stable from 100 blocks to 300 blocks at less than 1GB.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Before this commit, KernelPtr objects created through its _from_ptr constructor would not be owned by the user. This is incorrect, in most cases the user is responsible for freeing up these pointers. This bug was introduced by #3, and causes all objects created through the
KernelPtr._from_ptrconstructor to not be freed by the user, leading to memory leaks such as reported in #9.Only in the case of callbacks (e.g. for notifications and logging) would the pointer not be owned by the user. However, this usage is currently not implemented.
Fix this by making all KernelPtr objects owned by the user by default (as it was before #3), while still allowing this to be overridden in the future (e.g. when notifications are more fully implemented) through the _from_ptr constructor.
Fixes #9