Skip to content

Change DataInfo such that it doesn't keep link to parent - #6279

Closed
mhvk wants to merge 2 commits into
astropy:masterfrom
mhvk:data-info-memory-leak-2
Closed

mhvk wants to merge 2 commits into
astropy:masterfrom
mhvk:data-info-memory-leak-2

Conversation

@mhvk

@mhvk mhvk commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

This is an alternative to #6277 for fixing #6276. Instead of making _parent a weak reference, it no longer stores the info instance on the parent, but instead just stores the information there. So, an instance is recreated every time info is accessed, but it is quickly updated. It has the advantage that nothing needs to be adjusted, but perhaps can be done better. cc @taldcroft, @MSeifert04 (who seems to understand reference counting better...)

p.s. I'm not 100% sure we use the descriptor interface optimally...

@mhvk

mhvk commented Jun 26, 2017

Copy link
Copy Markdown
Contributor Author

Closing since I cannot get this to work well -- too fragile.

@mhvk mhvk closed this Jun 26, 2017
@taldcroft

Copy link
Copy Markdown
Member

I was just looking at this. I can't understand why it is failing so weirdly in CI. I agree in principle with the basic idea ... except that it seems to add complexity to something that is already pretty difficult for me to understand (and I wrote the original!).

@mhvk

mhvk commented Jun 26, 2017

Copy link
Copy Markdown
Contributor Author

The failure is because if you set an attribute that also exists on the class, it gets stored in _info, but then when you get the attribute, it gets found on the class before __getattr__ has a chance...

@taldcroft

Copy link
Copy Markdown
Member

Ah right, then you need to implement __getattribute__ and it gets a little wonky?

@mhvk

mhvk commented Jun 26, 2017

Copy link
Copy Markdown
Contributor Author

Agreed, I didn't want to go the __getattribute__ route - as you said, this code is tricky enough as is. There must be a better way...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants