Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

BUG io: fix file descriptor closing for netcdf variables #2792

Closed
wants to merge 1 commit into from

4 participants

@loujine

File descriptors created for each Variable stayed open when
using mmap

Closes gh-1555

@loujine loujine BUG io: fix file descriptor closing for netcdf variables
File descriptors created for each Variable stayed open when
using mmap

Closes gh-1555
2956315
@rgommers
Owner

Nice catch. This probably fixes a persistent warning in the test suite about not closing a file. If that's the case (not tested yet), that serves as an OK regression test.

@matthew-brett
Collaborator

Still, I guess we ignored that warning for a long time, so it would be good to have an explicit test for this change, to make sure it doesn't come back.

@rgommers
Owner

Checked, there's actually no warning for this in Python 3. I was thinking that checking this would be slow, but it's actually OK. So a test like this can be added:

fname = '../scipy/scipy/io/tests/data/example_1.nc'
vars = []
for i in range(1100):
    f = spio.netcdf_file(fname, mmap=True)
    vars.append(f.variables['lat'])
    f.close()
@pv
Owner
pv commented

This probably should use weakrefs, otherwise all the mmap instances are kept around until the file is closed, which may cause problems.

@rgommers
Owner

@pv just tried to follow your suggestion, but ran into http://bugs.python.org/issue4885.

Proposed test added in https://github.com/rgommers/scipy/tree/pr/2792.

@matthew-brett
Collaborator

Ralf - sorry to be dumb - I'm jet-lagged - but where is the test exactly? Should it show up in this thread as a commit?

@rgommers
Owner

No, I can't push a commit to someone else's PR. It's in the pr/2792 branch of my fork, linked above.

Commit: rgommers@cda8208

@matthew-brett
Collaborator

Aha - thanks. Did you make a pull request to this branch? Or do you prefer to merge this one and then yours?

@rgommers
Owner

I usually (if my additions are simple and reviewed anyway) just merge mine which includes this one. Or I can submit a new PR and close this one. Two separate merges is no good.

@matthew-brett
Collaborator

Both commits look OK to me.

@rgommers rgommers referenced this pull request from a commit
@rgommers rgommers Merge branch 'pr/2792' into master.
Reviewed as #2792
b6c937e
@rgommers
Owner

Thanks Matthew, merged in b6c937e.

@rgommers rgommers closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2013
  1. @loujine

    BUG io: fix file descriptor closing for netcdf variables

    loujine authored
    File descriptors created for each Variable stayed open when
    using mmap
    
    Closes gh-1555
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 1 deletion.
  1. +6 −1 scipy/io/netcdf.py
View
7 scipy/io/netcdf.py
@@ -43,7 +43,6 @@
from scipy.lib.six import integer_types
-
ABSENT = b'\x00\x00\x00\x00\x00\x00\x00\x00'
ZERO = b'\x00\x00\x00\x00'
NC_BYTE = b'\x00\x00\x00\x01'
@@ -199,6 +198,7 @@ def __init__(self, filename, mode='r', mmap=None, version=1):
if mmap is None:
mmap = True
self.use_mmap = mmap
+ self._fds = []
self.version_byte = version
if not mode in 'rw':
@@ -231,6 +231,8 @@ def close(self):
if not self.fp.closed:
try:
self.flush()
+ for mmap_fd in self._fds:
+ mmap_fd.close()
finally:
self.fp.close()
__del__ = close
@@ -591,6 +593,7 @@ def _read_var_array(self):
mm = mmap(self.fp.fileno(), begin_+a_size, access=ACCESS_READ)
data = ndarray.__new__(ndarray, shape, dtype=dtype_,
buffer=mm, offset=begin_, order=0)
+ self._fds.append(mm)
else:
pos = self.fp.tell()
self.fp.seek(begin_)
@@ -613,6 +616,7 @@ def _read_var_array(self):
mm = mmap(self.fp.fileno(), begin+self._recs*self._recsize, access=ACCESS_READ)
rec_array = ndarray.__new__(ndarray, (self._recs,), dtype=dtypes,
buffer=mm, offset=begin, order=0)
+ self._fds.append(mm)
else:
pos = self.fp.tell()
self.fp.seek(begin)
@@ -623,6 +627,7 @@ def _read_var_array(self):
for var in rec_vars:
self.variables[var].__dict__['data'] = rec_array[var]
+
def _read_var(self):
name = asstr(self._unpack_string())
dimensions = []
Something went wrong with that request. Please try again.