ZipFile.open() should not reopen the underlying file

@loewis

BPO	14099
Nosy	@loewis, @pitrou, @larryhastings, @benjaminp, @mcepl, @merwok, @bitdancer, @dw, @serhiy-storchaka, @eryksun
Files	Proposed-fix-of-issue14099.patch: proposed fix, against latest source tree Proposed-fix-of-issue14099-second.patch: Updated patch zipfile_concurrent_read_1.diff: Draft patch (from dw) against 524a004e93dd zipfile_share_file.patch bench_zip.py zipfile_tellable.patch: Support tellable but unseekable files in ZipFile.writestr() zipfile_threadsafe.patch: Makes ZipFile threadsafe

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-01-26.12:05:57.524>
created_at = <Date 2012-02-23.10:46:31.740>
labels = ['type-bug', 'library']
title = 'ZipFile.open() should not reopen the underlying file'
updated_at = <Date 2015-02-05.21:24:35.655>
user = 'https://bugs.python.org/kasal'

bugs.python.org fields:

activity = <Date 2015-02-05.21:24:35.655>
actor = 'eryksun'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-01-26.12:05:57.524>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2012-02-23.10:46:31.740>
creator = 'kasal'
dependencies = []
files = ['24617', '24624', '37191', '37197', '37198', '37732', '37733']
hgrepos = []
issue_num = 14099
keywords = ['patch']
message_count = 30.0
messages = ['154058', '154077', '154110', '154123', '176844', '176853', '176855', '176857', '176858', '176859', '231132', '231181', '231191', '231473', '231475', '231918', '232032', '232035', '232048', '232055', '232068', '232070', '233553', '234142', '234143', '234146', '234735', '234736', '235183', '235453']
nosy_count = 17.0
nosy_names = ['loewis', 'alanmcintyre', 'pitrou', 'ocean-city', 'larry', 'benjamin.peterson', 'mcepl', 'durin42', 'eric.araujo', 'Arfrever', 'r.david.murray', 'python-dev', 'Matt.Mackall', 'kasal', 'dw', 'serhiy.storchaka', 'eryksun']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14099'
versions = ['Python 3.5']

When a file inside a zip is open, the underlying zip file is open again.
(Unless the file name is unknown, because the ZipFile object was created with fp only.)

This design is incorrect, insecure, and ineffective:

the reopen uses the same string as file name, but on unix-like systems that file name may no longer exist, or may point to a different file
opening n files from the same zip archive consumes n OS file descriptors, wasting resources

I believe that the parent ZipFile object and all the child ZipExtFile objects should keep the same fp. The last one would close it.

I'm working on a patch currently.

Attached please find a patch that fixes this issue by reusing the original fp from ZipFile object.

Two of the test cases attempted to read a file from a zip as soon as write() was called. I believe that this is not correct usage: zip file is not even fully written to disk at that stage!
So I took the liberty to change these two test cases so that they first write the file and then read it.

Let me thank to Martin Sikora for discovering the issue and to Matej Cepl for testing it on current source tree.

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?

Attached please find a second iteration of the fix.
This time the signature of ZipExtFile is kept backward compatible, with one new parameter added.

This issue looks as a duplicate or a superseder of bpo-10631. See also bpo-16569.

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.

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 bpo-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.

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.

I'm not sure when I'll get to this, sorry.
Hopefully sometime soon.

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 (bpo-10376, bpo-16304). 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 (bpo-8745).

I want to see and test an updated patch.

Sorry, not bpo-16304, but bpo-16034. The commit messages were wrong.

Sounds great :)

New changeset c2c4cde55f6f by Serhiy Storchaka in branch '2.7':
Issue bpo-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 bpo-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 bpo-14099: ZipFile.open() no longer reopen the underlying file. Objects
https://hg.python.org/cpython/rev/334c01aa7f93

Thanks Stepan for the idea.

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.

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.

Adding locks almost not affects performance, because reads are done by relative large chunks and locking overhead is small.

Navigation Menu

Repository navigation

ZipFile.open() should not reopen the underlying file #58307

10 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

ZipFile.open() should not reopen the underlying file #58307

Description

Activity

kasal commented on Feb 23, 2012

kasal commented on Feb 23, 2012

merwok commented on Feb 24, 2012

kasal commented on Feb 24, 2012

serhiy-storchaka commented on Dec 3, 2012

kasal commented on Dec 3, 2012

serhiy-storchaka commented on Dec 3, 2012

kasal commented on Dec 3, 2012

serhiy-storchaka commented on Dec 3, 2012

serhiy-storchaka commented on Dec 3, 2012

10 remaining items

dw commented on Dec 2, 2014

python-dev commented on Dec 3, 2014

serhiy-storchaka commented on Dec 3, 2014

MattMackall commented on Jan 6, 2015

serhiy-storchaka commented on Jan 16, 2015

serhiy-storchaka commented on Jan 16, 2015

serhiy-storchaka commented on Jan 16, 2015

python-dev commented on Jan 26, 2015

serhiy-storchaka commented on Jan 26, 2015

python-dev commented on Feb 1, 2015

eryksun commented on Feb 5, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions