Skip to content

PR #4539 backport to v1.0.x with Python 2.6 support - #4540

Merged
embray merged 6 commits into
astropy:v1.0.xfrom
embray:fits/pr-4539
Jan 28, 2016
Merged

embray merged 6 commits into
astropy:v1.0.xfrom
embray:fits/pr-4539

Conversation

@embray

@embray embray commented Jan 28, 2016

Copy link
Copy Markdown
Member

This backports #4539 to the v1.0.x and v1.1.x branch (the latter can be done via cherry-pick). This required some extra work to support Python 2.6, which is still supported on those branches.

as explained here:
https://github.com/astropy/astropy/pull/520/files#diff-8ba48829876ba755e67054a1a90ec124R2367

I will add more in-line documentation of this in a subsequent commit.
But basically what's going on is this:  Whenever a Column object's
.array object is assigned to, it actually walks that array's bases to
check for the reference cycle, by seeing if one of those bases is a
FITS_rec instance with the present Column in its ColDefs.  If so, rather
than creating a hard reference it stores a weak reference to the
original FITS_rec, and the .array property returns the array via that
weak reference.

That way the reference cycle never gets created in the first place.

On the FITS_rec end, it actually also keeps a set of weak references to
all Column objects associated with that FITS_rec through its ._coldefs.
When FITS_rec.__del__ is called (because its reference count has gone to
zero) it deletes self._coldefs.  If that ColDefs object has no
references outside the FITS_rec (i.e. a user has not pointed a variable
to it), it and all Columns it contains are properly deleted.  This also
removes the column from the FITS_rec's _col_weakrefs set.

However, if there are still references to the Column (again, by a user
for example) then the data for that array is copied (maybe instead we
can just restore the reference to the original FITS_rec?), so the user
can continue to access that data.  This is allowed because by the time
FITS_rec.__del__ is called its underlying memory has not been freed yet.
The one exception to this was where data.deleter closed any mmap
underlying the data.  But I fixed the order of things so that the mmap
is still available to perform that copy.

This is all probably still fairly fragile, but in the vast majority of
cases it fixes the chance for any reference leaks.
…ibiting the reference leak. Remove the previous note in the test for astropy#520 about the existing reference leak, since it *shouldn't* apply any longer.
… make sure that when a weakref is assigned for Column.array, that the original .array contents, if any, are dropped.
@embray

embray commented Jan 28, 2016

Copy link
Copy Markdown
Member Author

Just the doc build failed due to unrelated warnings (fixed in ac1cca8 which I have no backported).

embray added a commit that referenced this pull request Jan 28, 2016
PR #4539 backport to v1.0.x with Python 2.6 support
@embray
embray merged commit 030f87b into astropy:v1.0.x Jan 28, 2016
@embray
embray deleted the fits/pr-4539 branch January 28, 2016 21:35
embray added a commit that referenced this pull request Jan 28, 2016
PR #4539 backport to v1.0.x with Python 2.6 support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant