Opened 5 years ago

Closed 4 years ago

Last modified 3 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 Branch: branches/streaming-zipfile-4035
(diff, github, buildbot, log)
Author: lvh Launchpad Bug:

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 aruse 4 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 aruse 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by exarkun

See also #3666.

comment:2 Changed 5 years ago by glyph

  • Keywords easy added
  • Owner changed from glyph to kmike
  • Summary changed from deprecate or improve zipstream to 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 5 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 4 years ago by aruse

  • Owner changed from kmike to aruse
  • Status changed from new to assigned

Changed 4 years ago by aruse

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

comment:5 Changed 4 years ago by aruse

  • Keywords review added
  • Owner aruse deleted
  • Status changed from assigned to new

comment:6 Changed 4 years ago by aruse

  • Keywords changed from easy, review to easy review

comment:7 Changed 4 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to aruse

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

Changed 4 years ago by aruse

comment:8 Changed 4 years ago by aruse

  • Keywords review added
  • Owner aruse 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 4 years ago by lvh

  • Owner set to lvh
  • Status changed from new to assigned

comment:10 Changed 4 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 4 years ago by lvh

  • Keywords review removed
  • Status changed from assigned to new

Looks great, merging (with previous caveat).

comment:12 Changed 4 years ago by lvh

  • Author set to lvh
  • Branch set to branches/streaming-zipfile-4035

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

comment:14 Changed 4 years ago by lvh

  • Resolution set to fixed
  • Status changed from new to closed

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

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

comment:15 Changed 3 years ago by exarkun

#4498 was a duplicate of this.

Note: See TracTickets for help on using tickets.