ZipFile.open() should not reopen the underlying file

[kasal]

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']

[kasal]

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.

[kasal]

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.

[merwok]

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?

[kasal]

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

[serhiy-storchaka]

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.

[kasal]

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.

[serhiy-storchaka]

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.

[kasal]

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

[serhiy-storchaka]

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.

[serhiy-storchaka]

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