Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix ticket #2178: "own" (to close) file handles in load() only if they were not opened before #328

Merged
merged 7 commits into from

3 participants

@yarikoptic

See http://projects.scipy.org/numpy/ticket/2178 for more information

Also I am not 100% sure if I have not introduced leaking of file descriptors somehow (although did run that specific added unittest 10000s of time and everything was legit)... otherwise -- now it passes all old tests and the new one

yarikoptic added some commits
@yarikoptic yarikoptic BUG: do not "own" the FID for GzipFile and file if provided to load a…
…lready opened (ticket #2178)

Also made all assignments of own_file go in pair with assignments to fid to make things clearer
4df2444
@yarikoptic yarikoptic ENH: unittest for preceeding commit fixing #2178 0e3a3d9
@travisbot

This pull request fails (merged 0e3a3d9 into e15d0bd).

@travisbot

This pull request fails (merged 4219824 into e15d0bd).

@yarikoptic

hm, the last failure of @travisbot looks like a bug on its own, or I am wrong?

3438Traceback (most recent call last):
3439 File "/home/vagrant/virtualenv/python3.2/lib/python3.2/site-packages/numpy/lib/tests/test_io.py", line 178, in test_not_closing_opened_fid
3440 np.savez(fp, data='LOVELY LOAD')
3441 File "/home/vagrant/virtualenv/python3.2/lib/python3.2/site-packages/numpy/lib/npyio.py", line 530, in savez
3442 _savez(file, args, kwds, False)
3443 File "/home/vagrant/virtualenv/python3.2/lib/python3.2/site-packages/numpy/lib/npyio.py", line 592, in _savez
3444 zip.write(tmpfile, arcname=fname)
3445 File "/usr/lib/python3.2/zipfile.py", line 1111, in write
3446 self.fp.write(zinfo.FileHeader())
3447TypeError: must be str, not bytes
numpy/lib/npyio.py
((9 lines not shown))
elif isinstance(file, gzip.GzipFile):
+ # we were provided an existing handle which we should close
+ # only if it was closed already
@njsmith Owner
njsmith added a note

This comment is very confusing... why do we need to close handles that are already closed?

Right! For some reason I thought that may be numpy/gzip/... somehow could reopen this closed handle later on to perform the operation.
Since that is not possible (?) per se -- I removed majority of the changes and that boiled down to smaller fix (should I squash?), but because original commit which introduced all this logic 8630830 was not accompanied by a unittest I was not sure if I am creating a regression here. I have added a rudimentary unittest for it (I will push shortly as the part of this pull request) but I failed to trigger it on my box even if I removed explicit .close() for own_fid = True. So -- something for travis to check I guess

btw, own_fid is somewhat an incorrect name here since we are not operating on FIDs but on handles, so own_fh or own_file (both used elsewhere in numpy) might be more appropriate

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

This pull request fails (merged 81a03cf into e15d0bd).

@yarikoptic

wow -- even original test now fails -- I was not careful enough in my local testing I guess... bringing back explicit own_fid = False for .npz

@travisbot

This pull request fails (merged c35c83c into e15d0bd).

@yarikoptic

ok -- works now -- only that python3-specific "TypeError: must be str, not bytes" which gets triggered by both added unittests and imho which smells like a bug in zipfile since FileHeader promises """Return the per-file header as a string."""

@njsmith
Owner

Yeah, maybe a bug should be filed with the stdlib... in the mean time we have to work around it somehow, though; can't merge a pull request with failing tests.

@yarikoptic

ok -- let me get a glimpse at that failure with python3

@yarikoptic yarikoptic BF(PY3): open file handles in tests in binary mode
otherwise zipfile of python3 gets confused to receive bytes for the header
whenever handle is opened for a text (unicode) file
613589e
@travisbot

This pull request fails (merged 613589e into e15d0bd).

@yarikoptic

this time it fails clearly not because of me ;)

15$ git checkout -qf 126c2cbbe8b5d1b680591d41e73c31385d435a11
16$ export TRAVIS_PYTHON_VERSION=3.2
17$ source ~/virtualenv/python3.2/bin/activate
18$ python --version
19Python 3.2.2
20$ pip --version
21pip 1.1 from /home/vagrant/virtualenv/python3.2/lib/python3.2/site-packages/pip-1.1-py3.2.egg (python 3.2)
24Executing your before_install (mkdir builds) took longer than 900 seconds and was terminated. Consider rewriting your stuff in AssemblyScript, we've heard it handles Web Scale™

25

26

27Done. Build script exited with: 1

and succeeded on other python3 boxes -- whoorray! ;)

@njsmith
Owner

Right, Travis-CI is kind of flaky like that...

Anyway, looks good -- tiny fix, tests pass, yay.

@njsmith njsmith merged commit 3b9a0fe into numpy:master
@teoliphant teoliphant referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 2, 2012
  1. @yarikoptic

    BUG: do not "own" the FID for GzipFile and file if provided to load a…

    yarikoptic authored
    …lready opened (ticket #2178)
    
    Also made all assignments of own_file go in pair with assignments to fid to make things clearer
  2. @yarikoptic
  3. @yarikoptic
Commits on Jul 5, 2012
  1. @yarikoptic
  2. @yarikoptic
  3. @yarikoptic
Commits on Jul 6, 2012
  1. @yarikoptic

    BF(PY3): open file handles in tests in binary mode

    yarikoptic authored
    otherwise zipfile of python3 gets confused to receive bytes for the header
    whenever handle is opened for a text (unicode) file
This page is out of date. Refresh to see the latest.
Showing with 46 additions and 2 deletions.
  1. +1 −2  numpy/lib/npyio.py
  2. +45 −0 numpy/lib/tests/test_io.py
View
3  numpy/lib/npyio.py
@@ -359,7 +359,6 @@ def load(file, mmap_mode=None):
own_fid = True
elif isinstance(file, gzip.GzipFile):
fid = seek_gzip_factory(file)
- own_fid = True
else:
fid = file
@@ -371,7 +370,7 @@ def load(file, mmap_mode=None):
fid.seek(-N, 1) # back-up
if magic.startswith(_ZIP_PREFIX): # zip-file (assume .npz)
own_fid = False
- return NpzFile(fid, own_fid=True)
+ return NpzFile(fid, own_fid=own_fid)
elif magic == format.MAGIC_PREFIX: # .npy file
if mmap_mode:
return format.open_memmap(file, mode=mmap_mode)
View
45 numpy/lib/tests/test_io.py
@@ -167,6 +167,51 @@ def writer(error_list):
if errors:
raise AssertionError(errors)
+ def test_not_closing_opened_fid(self):
+ # Test that issue #2178 is fixed:
+ # verify could seek on 'loaded' file
+
+ fd, tmp = mkstemp(suffix='.npz')
+ os.close(fd)
+ try:
+ fp = open(tmp, 'wb')
+ np.savez(fp, data='LOVELY LOAD')
+ fp.close()
+
+ fp = open(tmp, 'rb', 10000)
+ fp.seek(0)
+ assert_(not fp.closed)
+ _ = np.load(fp)['data']
+ assert_(not fp.closed) # must not get closed by .load(opened fp)
+ fp.seek(0)
+ assert_(not fp.closed)
+
+ finally:
+ os.remove(tmp)
+
+ def test_closing_fid(self):
+ # Test that issue #1517 (too many opened files) remains closed
+ # It might be a "week" test since failed to get triggered on
+ # e.g. Debian sid of 2012 Jul 05 but was reported to
+ # trigger the failure on Ubuntu 10.04:
+ # http://projects.scipy.org/numpy/ticket/1517#comment:2
+ fd, tmp = mkstemp(suffix='.npz')
+ os.close(fd)
+
+ try:
+ fp = open(tmp, 'wb')
+ np.savez(fp, data='LOVELY LOAD')
+ fp.close()
+
+ for i in range(1, 1025):
+ try:
+ np.load(tmp)["data"]
+ except Exception, e:
+ raise AssertionError("Failed to load data from a file: %s" % e)
+ finally:
+ os.remove(tmp)
+
+
class TestSaveTxt(TestCase):
def test_array(self):
a = np.array([[1, 2], [3, 4]], float)
Something went wrong with that request. Please try again.