Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#4035 enhancement closed fixed (fixed)

use python 2.6's streaming zipfile support instead of our own zipstream module

Reported by: kmike Owned by: lvh
Priority: normal Milestone:
Component: core Keywords: easy
Cc: kmike, Thijs Triemstra Branch: branches/streaming-zipfile-4035
branch-diff, diff-cov, branch-cov, buildbot
Author: lvh

Description

As I can see the goal of zipstream is to provide support for an incremental approach to unzipping files.

But standard zipfile has this support since python 2.6. ZipFile?.open() method returns file-like object which supports 'read' method with buffer size in parameters.

Implementation in zipfile is better than in zipstream. At least it works a lot faster because it don't have 'self.buffer = self.buffer[n:]' code which kills performance on big (even tens of Kb's) buffer sizes like zipstream version does.

I think is better to deprecate zipstream and copy implementation from new zipfile. Or make zipstream a backported zipfile wrapper.

Attachments (2)

4035.patch​ (1.6 KB) - added by Andy Ruse 7 years ago.
Modified zippath module to use the standard zipfile module if version is >= 2.6. Older versions will still use zipstream.
4035.patch2​ (1.7 KB) - added by Andy Ruse 7 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by Jean-Paul Calderone

See also #3666.

comment:2 Changed 8 years ago by Glyph

Keywords: easy added
Owner: changed from Glyph to kmike
Summary: deprecate or improve zipstream → use python 2.6's streaming zipfile support instead of our own zipstream module

I agree, it would be good to use the improved implementation on python 2.6 (and using a direct backport is better, because then we only have one bit of code to maintain).

comment:3 Changed 8 years ago by Glyph

Please note that ZipPath.open is the main use-case for this code. Really, the whole module should have been an implementation detail of ZipPath in the first place.

comment:4 Changed 7 years ago by Andy Ruse

Owner: changed from kmike to Andy Ruse
Status: new → assigned

Changed 7 years ago by Andy Ruse

Attachment: 4035.patch​ added

Modified zippath module to use the standard zipfile module if version is >= 2.6. Older versions will still use zipstream.

comment:5 Changed 7 years ago by Andy Ruse

Keywords: review added
Owner: Andy Ruse deleted
Status: assigned → new

comment:6 Changed 7 years ago by Andy Ruse

Keywords: easy, review → easy review

comment:7 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to Andy Ruse

This patch is missing tests and a news file, see TwistedDevelopment

Changed 7 years ago by Andy Ruse

Attachment: 4035.patch2​ added

comment:8 Changed 7 years ago by Andy Ruse

Keywords: review added
Owner: Andy Ruse deleted

I didn't write any new tests because the existing tests already cover the code that was modified. I've attached a new patch with a news file.

comment:9 Changed 7 years ago by lvh

Owner: set to lvh
Status: new → assigned

comment:10 Changed 7 years ago by lvh

(For other people watching this: please don't name your N-th patch blah.patchN, because trac no longer recognizes your thing as a patch)

comment:11 Changed 7 years ago by lvh

Keywords: review removed
Status: assigned → new

Looks great, merging (with previous caveat).

comment:12 Changed 7 years ago by lvh

Author: → lvh
Branch: → branches/streaming-zipfile-4035

(In [31050]) Branching to 'streaming-zipfile-4035'

comment:14 Changed 7 years ago by lvh

Resolution: → fixed
Status: new → closed

(In [31053]) Use python 2.6's streaming zipfile support instead of zipstream

Author: aruse Reviewer: lvh, thijs Fixes: #4035

comment:15 Changed 7 years ago by Jean-Paul Calderone

#4498 was a duplicate of this.

Note: See TracTickets for help on using tickets.