Variables don't work if Dataset is garbage collected

[takluyver]

This code:

from netCDF4 import Dataset

filename = '/home/takluyver/Code/xray/test/data/example_1.nc'
d = Dataset(filename, mode='r', clobber=True, diskless=False, persist=False, format='NETCDF4')
v = d.variables['rh']
del d
print(v.shape)

Fails with:

Traceback (most recent call last):
  File "idbug2.py", line 13, in <module>
    print(v.shape)
  File "netCDF4.pyx", line 2556, in netCDF4.Variable.shape.__get__ (netCDF4.c:31870)
    msg = 'filling on, default _FillValue of %s used\n' % fillval
  File "netCDF4.pyx", line 2527, in netCDF4.Variable._getdims (netCDF4.c:31498)
    else:
RuntimeError: NetCDF: Not a valid ID

I believe I've traced this to PR #219. Variables no longer hold a reference to their group, so if the group and the dataset are garbage collected, the variable becomes unusable.

CC @shoyer - this is the underlying cause of the error in xray we discussed with netCDF 1.0.9.

[jswhit]

This seems like the expected (correct) behavior - or am I missing something?

[shoyer]

I think the issue is that dataset objects are being garbage collected and closed even though we are still referencing netcdf4 variables that refer to them.

On Mon, May 5, 2014 at 7:37 PM, Jeff Whitaker notifications@github.com
wrote:

This seems like the expected (correct) behavior - or am I missing something?

Reply to this email directly or view it on GitHub:
#251 (comment)

[takluyver]

If this is correct, xray will just have to keep a reference to the netCDF4 Dataset instance as long as it's using any variables (I did try that, and hit a segfault). I mostly wanted to document the fact that something downstream was relying on this not happening.

Note that the del d is contrived to make the example simple - it will normally be garbage collected when a function extracts a variable from the dataset and returns without storing the Dataset instance anywhere.

[shoyer]

I also deserve some responsibility for this one. I was pushing for releasing a new version of netCDF4, and I hadn't even tested it against my project that relies on it! Next time I'll definitely run those tests first.

On Mon, May 5, 2014 at 7:47 PM, Thomas Kluyver notifications@github.com
wrote:

If this is correct, xray will just have to keep a reference to the netCDF4 Dataset instance as long as it's using any variables (I did try that, and hit a segfault). I mostly wanted to document the fact that something downstream was relying on this not happening.

Note that the del d is contrived to make the example simple - it will normally be garbage collected when a function extracts a variable from the dataset and returns without storing the Dataset instance anywhere.

Reply to this email directly or view it on GitHub:
#251 (comment)

[jswhit]

I'm surprised that the Dataset instance would be garbage collected if there were still a reference a variable associated with it. Can you demonstrate this with a simple script? If so, maybe we can figure out a way we to prevent this from happening.

[jswhit]

I guess this is why (from Python weakref docs):

A weak reference to an object is not enough to keep the object alive: when the only remaining references to a referent are weak references, garbage collection is free to destroy the referent and reuse its memory for something else.

[takluyver]

That is precisely the point of weakrefs - to hold a reference that won't keep the object alive if it's the last remaining reference.

As for a script demonstrating the problem, see my code sample above. It should work with any netCDF file.

[jswhit]

Yes, but in your script the dataset is not garbage collected, but forcibly deleted.

[takluyver]

You can't forcibly delete things in Python - del deletes a reference to the object, and if that was the last remaining reference, the object is garbage collected.

You could achieve the same like this:

def get_var(filename, varname):
    d = Dataset(filename)
    return d.variables[varname]
filename = '/home/takluyver/Code/xray/test/data/example_1.nc'
print(get_var(filename, 'rh').shape)

[jswhit]

Should we have variables keep a reference to the Dataset of Group to which they belong? This was the previous behaviour, but this was changed by #218 so that circular references would not keep the Dataset instances from being garbage collected.

I think I like the current behavior, it seems more intuitive to me that the Dataset instance should be garbage collected when it goes out of scope (even if there are references to variable in it still alive).

The alternative is to revert to the old behavior, and live with the circular references.

[shoyer]

Python itself handles garbage collecting circular references just fine. I'm guessing there is some reason why this is not the case for the netCDF4 Cython module?

[jswhit]

This example was the motivation for changing to weakrefs (issue #218):

import netCDF4
def test():
    nc = netCDF4.Dataset('junk.nc', mode='w')
    dim = nc.createDimension('fred', 2000)
test()
test()

The second call to test triggered an error, since the file was still open even though both the dataset and the dimension instances were out of scope. With 1.0.9, no error results since dim only holds a weakref to the dataset, and the dataset is garbage collected when it goes out of scope.

So, it doesn't look to me like circular references are always garbage collected for Cython extension modules.

Perhaps one solution is to flip it around so that the Dataset instances hold weakrefs to variables and dimensions, and variables/dimensions have strong refs to the Dataset?

[takluyver]

AIUI, most circular references will be garbage collected, but not necessarily immediately: the garbage collector only checks for unreachable circular references at certain intervals (whereas objects without circular references are cleaned up when their reference count hits 0).

However, there may also be objects which it doesn't know how to clean up safely when in reference cycles. Prior to Python 3.4, that was anything with a __del__ method. From Python 3.4, it's only C extension objects with the equivalent slot (tp_del) - but I think that might be happening in this case, if Cython converts the __dealloc__ method to tp_del.

I'd be inclined to have objects from the Dataset hold a reference to the dataset, and require explicitly closing it if you need to reopen the same file. But then, I've hit this problem and not the other one.

[shoyer]

Weak references do not suffice to stop garage collection so I doubt that is a good option, since then the Variable objects wouldn't be guaranteed to stick around at all.

In an ideal world, we could simply support multiple connections to the same file (this is the apparent case with python's built-in open).

In my opinion, the right solution is to revert to the previous behavior and encourage using a context manager for creating datasets:

import netCDF4
import sys

def test():
    with netCDF4.Dataset('junk.nc', mode='w', format='NETCDF4') as nc:
        dim = nc.createDimension('fred', 2000)

test()
test()

I think it's very weird to not keep a dataset around when it has variables referencing it. The current solution is not guaranteed to work for arbitrary python interpreters, but depends on implementation details of CPython.