Changeset 844

Timestamp:

03/25/11 16:46:17 (14 months ago)

Author:

stsci_embray

Message:

This should solve the memory leak reported in ticket #49.

After trying several options, I found this to be the simplest solution
requiring the fewest changes that works in the most scenarios.

This solution replaces the internal FITS_rec._coldefs with
FITS_rec.__coldefs which may be a weak reference. FITS_rec._coldefs is
now a property that either returns FITS_rec.__coldefs directly, or, if it's
a weakref, dereferences it and returns the hard reference. Every assignment
to FITS_rec._coldefs that created a reference cycle has been replaced by
assigning FITS_rec._coldefs with a weak reference. But it doesn't have
to be a weakref either. Making FITS_rec._coldefs into a property makes this
transparent.

Another possibility, rather than making _coldefs into a property, would have
been to use weakref.proxy(), but there was a scenario where this wouldn't
have worked:

In #44 I added support for a FITS_rec.columns attribute that gives users
more obvious access to the ColDefs object underlying a FITS_rec (some
users rightly stayed away from using FITS_rec._coldefs directly). I wanted
to preserve this functionality. Making FITS_rec._coldefs a weakref proxy
would have had suprising results for users. For example.

>>> a = pyfits.new_table(...)
>>> c = a.data.columns  # I may want to do something with this later
>>> del a # I don't care about a, or I carelessly let it go out of scope
>>> print c
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ReferenceError: weakly-referenced object no longer exists
>>> # Oops!  Where did my ColDefs go?

If FITS_rec._coldefs were a proxy there would be no easy way to prevent
this, since I don't think it's possible to get a hard reference directly from
a proxy (without some sort of ugly hack).

So this solution is overall pretty transparent to both users and to most
internal code as well.

Files:

• trunk/lib/core.py (modified) (12 diffs)

trunk/lib/core.py

@@ -4505 +4505 @@
                 self._file.seek(self._datLoc)
                 data = GroupData(_get_tbdata(self))
-                data.columns = data._coldefs = self.columns
+                data._coldefs = weakref.ref(self.columns)
                 data.formats = self.columns.formats
                 data.parnames = self.columns._pnames
@@ -5667 +5667 @@
                                       names=tmp.names, shape=nrows))
 
-    hdu.data._coldefs = hdu.columns
+    hdu.data._coldefs = weakref.ref(hdu.columns)
     hdu.data.formats = hdu.columns.formats
 
@@ -5887 +5887 @@
         self._nfields = len(self.dtype.names)
         self._convert = [None]*len(self.dtype.names)
-        self.columns = self._coldefs = None
+        self.__coldefs = None
         self._gap = 0
         self.names = list(self.dtype.names)
@@ -5902 +5902 @@
         if isinstance(obj, FITS_rec):
             self._convert = obj._convert
-            self.columns = self._coldefs = obj._coldefs
+            self.__coldefs = obj.__coldefs
             self._nfields = obj._nfields
             self.names = obj.names
@@ -5917 +5917 @@
             self._file = getattr(obj,'_file', None)
 
-            self.columns = self._coldefs = None
+            self.__coldefs = None
             self._gap = 0
             self.names = list(obj.dtype.names)
@@ -5923 +5923 @@
             self.formats = None
 
-            attrs=['_convert', '_coldefs', 'names', '_names', '_gap', 'formats']
+            attrs=['_convert', '__coldefs', 'names', '_names', '_gap',
+                   'formats']
             for attr in attrs:
                 if hasattr(obj, attr):
@@ -5931 +5932 @@
                     setattr(self, attr, value)
 
+    def columns(self):
+        """
+        Users may find it convenient to access the columns through the data
+        object, and might not notice they can since _coldefs is 'private'.  Go
+        ahead and give them read access to it.  (See ticket #44.)
+        """
+
+        return self._coldefs
+    columns = property(columns)
+
+    def _get_coldefs(self):
+        if isinstance(self.__coldefs, weakref.ReferenceType):
+            return self.__coldefs()
+        else:
+            return self.__coldefs
+
+    def _set_coldefs(self, val):
+        self.__coldefs = val
+    _coldefs = property(_get_coldefs, _set_coldefs)
+
     def _clone(self, shape):
         """
@@ -5951 +5972 @@
         elif isinstance(key, slice) or isinstance(key,np.ndarray):
             out = rec.recarray.__getitem__(self, key)
-            out.columns = out._coldefs = ColDefs(self._coldefs)
+            out._coldefs = ColDefs(self._coldefs)
             arrays = []
             out._convert = [None]*len(self.dtype.names)
@@ -6590 +6611 @@
                 raise TypeError, "table data has incorrect type"
 
+    #def __del__(self):
+        #for c in self.columns:
+        #    if hasattr(c, 'array'):
+        #        del c.array
+    #    if hasattr(self.data, '_coldefs'):
+    #        del self.data._coldefs
+
     def __getattr__(self, attr):
         """
@@ -6599 +6627 @@
                 self._file.seek(self._datLoc)
                 data = _get_tbdata(self)
-                data.columns = data._coldefs = self.columns
+                data._coldefs = weakref.ref(self.columns)
                 data.formats = self.columns.formats
 #                print "Got data?"
@@ -7995 +8023 @@
                                              formats=",".join(cols._recformats),
                                              names=cols.names, shape=nrows))
-            self.compData._coldefs = self.columns
+            self.compData._coldefs = weakref.ref(self.columns)
             self.compData.formats = self.columns.formats
 
@@ -11161 +11189 @@
     hdulist, _ext = _getext(filename, mode, *ext, **extkeys)
     hdu = hdulist[_ext]
+    # Before hdu goes out of scope, convert the hdu.data._coldefs weakref to a
+    # hard reference
+    if hasattr(hdu.data, '_coldefs'):
+        hdu.data._coldefs = hdu.columns
     _data = hdu.data
     if _data is None and isinstance(_ext, _Zero):