[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Change by Jason R. Coombs :


--
resolution:  -> fixed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

Fix merged into master. Please use zipp 3.2.0 on Python 3.9 and earlier for the 
improved behavior or report back here if a backport is needed (and why).

--
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.10 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:


New changeset ebbe8033b1c61854c4b623aaf9c3e170d179f875 by Jason R. Coombs in 
branch 'master':
bpo-40564: Avoid copying state from extant ZipFile. (GH-22371)
https://github.com/python/cpython/commit/ebbe8033b1c61854c4b623aaf9c3e170d179f875


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I've released zipp 3.2.0 that includes a fix for this issue (option 2). Please 
test it out.

Because this change affects the user's expectation about the effect of what's 
passed in, I'm hesitant to backport it to 3.8 and 3.9. It's too late to go into 
3.9.0, so the earliest it can appear is in 3.9.1.

Please advise - does the undesirable behavior warrant a bugfix backport, or is 
it sufficient to direct users impacted by this issue prior to Python 3.10 to 
use the `zipp` backport?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Change by Jason R. Coombs :


--
keywords: +patch
pull_requests: +21408
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/22371

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I've also created this alternative to option 2:

- https://github.com/jaraco/zipp/tree/bugfix/bpo-40564-mixin

This alternative approach uses a mix-in rather than subclasses, creating a new 
class on-demand. I was hoping this approach would enable just augmenting the 
instance rather than affecting `source.__class__`, but when I got to the 
implementation, I found that `source.__class__` had to be mutated regardless.

This approach does have an advantage over option 2 in that it would support 
other ZipFile subclasses for source. It has the disadvantage in that it creates 
a new subclass for every instance created.

I've thought about it a lot and while I'm not particularly happy with any of 
the approaches, I'm leaning toward option 2.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

In jaraco/zipp, I've implemented three of the options above:

- https://github.com/jaraco/zipp/tree/bugfix/bpo-40564-option-1
- https://github.com/jaraco/zipp/tree/bugfix/bpo-40564-option-2
- https://github.com/jaraco/zipp/tree/bugfix/bpo-40564-option-5

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

Implementing that last option:

```
diff --git a/zipp.py b/zipp.py
index 697d4a9..f244cba 100644
--- a/zipp.py
+++ b/zipp.py
@@ -111,6 +111,7 @@ class CompleteDirs(zipfile.ZipFile):
 
 res = cls.__new__(cls)
 vars(res).update(vars(source))
+res.close = lambda: None
 return res
 
 
```

Does appear to address the issue. I'm not super happy about the implementation, 
though.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I see a few options here:

- Implement CompleteDirs/FastLookup as adapters instead of subclasses, allowing 
the original ZipFile object to represent the state in a single place. This 
approach would likely be slower due to the indirection on all operations 
through the wrapper.
- Instead of constructing a new object and copying the state, CompleteDirs.make 
could mutate the existing ZipFile class, replacing `source.__class__` with the 
new class. This approach is messy and the caller would still need to be aware 
that this change could be applied to the zipfile object.
- Consider this use-case unsupported and document that any ZipFile object 
passed into Path is no longer viable and should not be referenced for another 
purpose.
- Eliminate the class-based performance optimizations and replace them with 
some functional/imperative form. This approach may provide less separation of 
concerns.
- Disable the close-on-delete behavior for the subclasses when state is copied 
from another.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I suspect the issue lies in how the CompleteDirs.make [replaces one instance 
with 
another](https://github.com/jaraco/zipp/blob/8ad959e61cd4be40baab93528775c2bf03c8f4e1/zipp.py#L112-L114)
 in order to provide a complete list of implied directories and to memoize the 
names lookup.

Because constructing a zipfile.Path object around a zipfile.ZipFile copies the 
underlying state, closing one will have the affect of closing the other.

I believe this issue is the same root issue as issue41350.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I am able to replicate the failure using the ondisk fixture:

```diff
diff --git a/test_zipp.py b/test_zipp.py
index a6fbf39..539d0a9 100644
--- a/test_zipp.py
+++ b/test_zipp.py
@@ -259,3 +259,11 @@ class TestPath(unittest.TestCase):
 def test_implied_dirs_performance(self):
 data = ['/'.join(string.ascii_lowercase + str(n)) for n in 
range(1)]
 zipp.CompleteDirs._implied_dirs(data)
+
+def test_read_does_not_close(self):
+for alpharep in self.zipfile_ondisk():
+with zipfile.ZipFile(alpharep) as file:
+for rep in range(2):
+p_ = zipp.Path(file, 'a.txt')
+with p_.open() as inner:
+print(list(inner))
```

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Jason R. Coombs]

Jason R. Coombs  added the comment:

I've attempted to replicate the issue in the 
[zipp](https://github.com/jaraco/zipp) test suite with this test:

```diff
diff --git a/test_zipp.py b/test_zipp.py
index a6fbf39..dc7c8aa 100644
--- a/test_zipp.py
+++ b/test_zipp.py
@@ -259,3 +259,10 @@ class TestPath(unittest.TestCase):
 def test_implied_dirs_performance(self):
 data = ['/'.join(string.ascii_lowercase + str(n)) for n in 
range(1)]
 zipp.CompleteDirs._implied_dirs(data)
+
+def test_read_does_not_close(self):
+for alpharep in self.zipfile_alpharep():
+for rep in range(2):
+p_ = zipp.Path(alpharep, 'a.txt')
+with p_.open() as inner:
+print(list(inner))
```

But the test passes.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

Bisection show this to be caused due to 
e5bd73632e77dc5ab0cab77e48e94ca5e354be8a . See also issue41640 for a similar 
report.

--
nosy: +xtreak

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Brett Cannon]

Change by Brett Cannon :


--
nosy: +jaraco

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40564] Using zipfile.Path with several files prematurely closes zip

[Xavier]

New submission from Xavier :

Given a .zip file with more than one inner file, when reading those inner files 
using zipfile.Path the zip module closes the .zip file prematurely, causing an 
error.

Given the following code (example zipfile is attached, but any should work).

with zipfile.ZipFile('foo.zip') as file:
for name in ['file-1.txt', 'file-2.txt']:
p = zipfile.Path(file, name)
with p.open() as inner:
print(list(inner)) # ValueError: seek of closed file

But the following code does not fail:

with zipfile.ZipFile('foo.zip') as file:
for name in ['file-1.txt', 'file-2.txt']:
with file.open(name) as inner:
print(list(inner)) # Works!


Python 3.8.2 macOS 10.15.4.

--
components: IO
files: zipfile.zip
messages: 368449
nosy: bustawin
priority: normal
severity: normal
status: open
title: Using zipfile.Path with several files prematurely closes zip
type: behavior
versions: Python 3.8
Added file: https://bugs.python.org/file49142/zipfile.zip

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com