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

DM-21314: Investigate GC problems with Storable #75

Merged
merged 2 commits into from Nov 5, 2019

Conversation

kfindeisen
Copy link
Member

This PR adds a custom holder type that blocks Python garbage collection of the associated object for the lifetime of the holder. It must be used for any class that supports Python subclasses.

Despite my best efforts, I only partially understand how PySharedPtr works. In particular, while I know that all C++ classes with direct Python subclasses need to use PySharedPtr as their holder type, I'm not sure if the same goes for their superclasses, or whether that depends on, e.g., the static types used in function signatures.

include/lsst/utils/python/PySharedPtr.h Outdated Show resolved Hide resolved
include/lsst/utils/python/PySharedPtr.h Outdated Show resolved Hide resolved
include/lsst/utils/python/PySharedPtr.h Outdated Show resolved Hide resolved
@TallJimbo
Copy link
Member

while I know that all C++ classes with direct Python subclasses need to use PySharedPtr as their holder type, I'm not sure if the same goes for their superclasses

This seems the same as the question of whether one could use unique_ptr as the holder for one class while using shared_ptr for another class with which it has any kind of inheritance relationship, and I do know the answer to that is definitely "no".

@kfindeisen
Copy link
Member Author

This seems the same as the question of whether one could use unique_ptr as the holder for one class while using shared_ptr for another class with which it has any kind of inheritance relationship, and I do know the answer to that is definitely "no".

Can you elaborate? I thought mixing unique_ptr or shared_ptr, or relying on their one-way implicit conversion, does not compile, whereas the only problem I've run into so far is that you don't get PySharedPtr's keep-alive reference if you declare a subclass with shared_ptr.

@TallJimbo
Copy link
Member

Can you elaborate? I thought mixing unique_ptr or shared_ptr, or relying on their one-way implicit conversion, does not compile, whereas the only problem I've run into so far is that you don't get PySharedPtr's keep-alive reference if you declare a subclass with shared_ptr.

Interesting that the holder types are treated differently w.r.t. whether the code compiles; I wonder if the implicit conversions are what make the difference. In any case, I don't have anything to add - I just wanted to make sure you knew what I knew about shared_ptr and unique_ptr, but you already knew that and then some.

@kfindeisen
Copy link
Member Author

kfindeisen commented Nov 5, 2019

Ok. I don't want to add PySharedPtr to every single subclass of Persistable if I don't have to, so I suggest we don't go that far until we know there's a problem.

This is a special holder type that interoperates with shared_ptr but
prevents certain memory errors when a Python class inherits from a
C++ class.
@kfindeisen kfindeisen merged commit 4f27db9 into master Nov 5, 2019
@kfindeisen kfindeisen deleted the tickets/DM-21314 branch November 5, 2019 19:37
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.

None yet

2 participants