[issue14099] ZipFile.open() should not reopen the underlying file
eryksun added the comment: The changeset from 03 Dec is in the Windows 2.7.9 release. Python 2.7.9 (default, Dec 10 2014, 12:28:03) [MSC v.1500 64 bit (AMD64)] on win32 Type help, copyright, credits or license for more information. import zipfile zipfile._SharedFile class zipfile._SharedFile at 0x00F707C8 -- nosy: +eryksun ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Roundup Robot added the comment: New changeset 4f96e9a8eee8 by Serhiy Storchaka in branch 'default': Don't seek to the start of the file when open ZipFile with the 'w' mode https://hg.python.org/cpython/rev/4f96e9a8eee8 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Roundup Robot added the comment: New changeset ae42c4576438 by Serhiy Storchaka in branch '2.7': Issue #14099: Backout changeset c2c4cde55f6f (except adapted tests). https://hg.python.org/cpython/rev/ae42c4576438 New changeset 680b47c96e08 by Serhiy Storchaka in branch '3.4': Issue #14099: Backout changeset e5bb3044402b (except adapted tests). https://hg.python.org/cpython/rev/680b47c96e08 New changeset 4973ccd46e32 by Serhiy Storchaka in branch 'default': Issue #14099: Writing to ZipFile and reading multiple ZipExtFiles is https://hg.python.org/cpython/rev/4973ccd46e32 New changeset 9cbf9f96920d by Serhiy Storchaka in branch 'default': Issue #14099: Restored support of writing ZIP files to tellable but https://hg.python.org/cpython/rev/9cbf9f96920d -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Sorry Stepan and David, but for this feature you need wait 3.5. -- resolution: - fixed stage: patch review - resolved status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Thank you for your report Matt. There is other problem. It is nowhere documented and newer granted and newer mentioned when ZipFile.open() was added, but file-like objects returned by ZipFile.open() could be read in different threads simultaneously. It makes sense because decompressors release GIL and parallel reading compressed file can has benefit. It is easy to fix both issues (I prefer to do this in separate paths), but due to the overall complexity it is safer to withdraw committed changes in maintained releases and apply additional patches only in default branch. -- stage: - patch review Added file: http://bugs.python.org/file37732/zipfile_tellable.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Changes by Serhiy Storchaka storch...@gmail.com: -- versions: -Python 2.7, Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Adding locks almost not affects performance, because reads are done by relative large chunks and locking overhead is small. -- Added file: http://bugs.python.org/file37733/zipfile_threadsafe.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: See also issue23252. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Changes by Augie Fackler li...@durin42.com: -- nosy: +durin42 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Changes by Serhiy Storchaka storch...@gmail.com: -- resolution: fixed - stage: resolved - status: closed - open ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Matt Mackall added the comment: The committed fix breaks Mercurial. http://bz.selenic.com/show_bug.cgi?id=4492 The underlying file-like object in our case is a wsgirequest but anything else trying to serve a dynamically-generated zip file on the web will probably die. We wrapped wsgirequest to support tell() many years ago probably copying someone else's hack, and it's worked fine across Python 2.4-2.7, but we fundamentally can't support all the new seek()s that were added here. -- nosy: +Matt.Mackall ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Benjamin Peterson added the comment: Okay for 2.7.10. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
David Wilson added the comment: Could we also make a small tweak to zipfile.rst indicating the new behaviour? I had made an initial attempt in my patch but wasn't particularly happy with the wording. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: How about just Objects returned by :meth:`.open` can operate independently of the ZipFile.? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
David Wilson added the comment: Sounds great :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Roundup Robot added the comment: New changeset c2c4cde55f6f by Serhiy Storchaka in branch '2.7': Issue #14099: ZipFile.open() no longer reopen the underlying file. Objects https://hg.python.org/cpython/rev/c2c4cde55f6f New changeset e5bb3044402b by Serhiy Storchaka in branch '3.4': Issue #14099: ZipFile.open() no longer reopen the underlying file. Objects https://hg.python.org/cpython/rev/e5bb3044402b New changeset 334c01aa7f93 by Serhiy Storchaka in branch 'default': Issue #14099: ZipFile.open() no longer reopen the underlying file. Objects https://hg.python.org/cpython/rev/334c01aa7f93 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Thanks Stepan for the idea. -- resolution: - fixed stage: patch review - resolved status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: What your thoughts Benjamin? Should this patch be applied to 2.7.10 (this is not critical for 2.7.9)? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: I hesitate about applying the patch to maintained releases. On one hand, besides interface (even non-documented details) left the same, the patch changes interiors too much for ordinal bug. I don't see how it can break something, but this doesn't guarantee that changes don't have unexpected effect. On other hand, this bug can be considered as security-related issue. Malicious local attacker could replace ZIP file between its open and read from it or between two reads, if he has write access to the directory containing ZIP file or there are symplinks under his control in ZIP file path. The danger of this may exceed hypothetical negative consequences of the applying of the patch. I appeal the matter to release managers. Should we apply this patch (the risk is pretty small) to 2.7 and 3.4? -- nosy: +benjamin.peterson, larry ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
David Wilson added the comment: While in spirit this is a bug fix, it's reasonably complex and affects a popular module -- I'm not sure it should be applied to 2.x, and probably not in a minor release of 3.x either. Would it make sense to include as part of 3.5? (That said, I'd love to see this fixed in 2.x ;)) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Thank you David for your benchmarks and patch. There are several backward compatibility issues with the reading from ZipFile opened for write and from closed ZipFile. This behavior is mostly undocumented (except the reading from closed ZipFile), but even our tests depend on it and changing it could break user code with good chance. Here is a patch which preserves current behavior. Added new tests to check this behavior explicitly. Other advantage of the patch is that it doesn't change the signature of ZipExtFile constructor at all. Benchmarks don't show stable significant difference between patched and unpatched versions. -- versions: +Python 3.5 -Python 3.2, Python 3.3 Added file: http://bugs.python.org/file37197/zipfile_share_file.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Changes by Serhiy Storchaka storch...@gmail.com: Added file: http://bugs.python.org/file37198/bench_zip.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
David Wilson added the comment: Hi Serhiy, Thanks for the new patch, it looks better than my attempt. :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
David Wilson added the comment:
Per my comment on issue16569, the overhead of performing one seek before each
(raw file data) read is quite minimal. I have attached a new (but incomplete)
patch, on which the following microbenchmarks are based. The patch is
essentially identical to Stepan's 2012 patch, except I haven't yet decided how
best to preserve the semantics of ZipFile.close().
my.zip is the same my.zip from issue22842. It contains 10,000 files each
containing 10 bytes over 2 lines.
my2.zip contains 8,000 files each containing the same copy of 64kb of
/dev/urandom output. The resulting ZIP is 500mb.
For each test, the first run is the existing zipfile module, and the second run
is with the patch. In summary:
* There is a 35% perf increase in str mode when handling many small files (on
OS X at least)
* There is a 6% perf decrease in file mode when handling small sequential reads.
* There is a 2.4% perf decrease in file mode when handling large sequential
reads.
From my reading of zipfile.py, it is clear there are _many_ ways to improve
its performance (probably starting with readline()), and rejection of a
functional fix should almost certainly be at the bottom of that list.
For each of the tests below, the functions used were:
def a():
Test concurrent line reads to a str mode ZipFile.
zf = zipfile.ZipFile('my2.zip')
members = [zf.open(n) for n in zf.namelist()]
for m in members:
m.readline()
for m in members:
m.readline()
def c():
Test sequential small reads to a str mode ZipFile.
zf = zipfile.ZipFile('my2.zip')
for name in zf.namelist():
with zf.open(name) as zfp:
zfp.read(1000)
def d():
Test sequential small reads to a file mode ZipFile.
fp = open('my2.zip', 'rb')
zf = zipfile.ZipFile(fp)
for name in zf.namelist():
with zf.open(name) as zfp:
zfp.read(1000)
def e():
Test sequential large reads to a file mode ZipFile.
fp = open('my2.zip', 'rb')
zf = zipfile.ZipFile(fp)
for name in zf.namelist():
with zf.open(name) as zfp:
zfp.read()
my.zip
$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.47 sec per loop
$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 950 msec per loop
---
$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 1.3 sec per loop
$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 865 msec per loop
---
$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 800 msec per loop
$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 851 msec per loop
my2.zip
$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.46 sec per loop
$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.16 sec per loop
---
$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 1.13 sec per loop
$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 892 msec per loop
---
$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 842 msec per loop
$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 882 msec per loop
---
$ python3.4 -m timeit -s 'import my' 'my.e()'
10 loops, best of 3: 1.65 sec per loop
$ python3.4 -m timeit -s 'import my' 'my.e()'
10 loops, best of 3: 1.69 sec per loop
--
nosy: +dw
Added file: http://bugs.python.org/file37191/zipfile_concurrent_read_1.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14099
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Changes by Serhiy Storchaka storch...@gmail.com: -- assignee: - serhiy.storchaka ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: This issue looks as a duplicate or a superseder of issue10631. See also issue16569. seek() for every read should significantly decrease performance. It may be worth to prohibit the simultaneous reading of different files from the archive. In any case the children counting in the patch looks doubtful. -- nosy: +Arfrever, loewis, ocean-city, pitrou, r.david.murray, serhiy.storchaka versions: +Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Stepan Kasal added the comment: Re: children counting You need to know the number of open children and whether the parent ZipFile object is still open. As soon as both all children and the parent ZipFile are closed, the underlying fp (corresponding to the file name given initially) shall be closed as well. The code submitted in the patch ensures that. But other implementations are possible. In any case, it is necessary to ensure that the children stay usable even if the parent ZipFile is closed, because of code like this: def datafile(self): with ZipFile(self.datafilezip, r) as f: return f.open(data.txt) This idiom currently works and should not be broken. Re: seek() The read can interfere not only with a parallel file expansion, but also with a ZipFile metadata read (user can list the contents of the zip again). Both of these would have to be forbidden by the documentation, and, ideally, also enforced. (As disscussed issue #16569) OTOH, zipfile.py is already slow, because the decompression is implemented in Python as interpreted code. I guess that the slowdown by seek() is neglectable compared to this. Also note that we most often seek to the current position; the OS should notice that and return swiftly. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: I think some benchmarks will needed to see how it will affect the performance. Please update your patch to current sources. The module code was changed last months. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Stepan Kasal added the comment: I'm not sure when I'll get to this, sorry. Hopefully sometime soon. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: This idiom currently works and should not be broken. Hmm. This seems doubtful to me, but if it is used, then I agree, it shouldn't be broken. I guess that the slowdown by seek() is neglectable compared to this. Even one function call can have effect on performance of short reads (issue10376, issue16304). Fortunately in this corner case the read buffer will be used. Also note that we most often seek to the current position; the OS should notice that and return swiftly. This may affect the buffered Python file (I did not check). The OS also doesn't notice this if the OS is Windows (issue8745). I want to see and test an updated patch. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Serhiy Storchaka added the comment: Sorry, not issue16304, but issue16034. The commit messages were wrong. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Stepan Kasal ka...@ucw.cz added the comment: Attached please find a second iteration of the fix. This time the signature of ZipExtFile is kept backward compatible, with one new parameter added. -- Added file: http://bugs.python.org/file24624/Proposed-fix-of-issue14099-second.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue14099] ZipFile.open() should not reopen the underlying file
Éric Araujo mer...@netwok.org added the comment: Thanks for the report and patch. I’m afraid changing the constructor signature is not an option, due to our backward compatibility policy. Do you think the bug can be fixed without changing the signature, or with new arguments added after the existing ones? -- nosy: +alanmcintyre, eric.araujo stage: - patch review title: zipfile: ZipFile.open() should not reopen the underlying file - ZipFile.open() should not reopen the underlying file type: resource usage - behavior versions: -Python 3.1 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14099 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
