[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.9 -Python 3.7, Python 3.8

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 8d62df60d8733d0fa9aee14ef746d0009a7a9726 by Serhiy Storchaka 
(Daniel Hillier) in branch 'master':
bpo-37523: Raise ValueError for I/O operations on a closed zipfile.ZipExtFile. 
(GH-14658)
https://github.com/python/cpython/commit/8d62df60d8733d0fa9aee14ef746d0009a7a9726


--

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Daniel Hillier]

Daniel Hillier  added the comment:

Good point. Thanks for the advice. I've updated it to use timeit. Does that 
give a better indication?

import zipfile

test_zip = "time_test.zip"
test_name = "test_name.txt"

# with zipfile.ZipFile(test_zip, "w") as zf:
# zf.writestr(test_name, "Hi there! " * 300)

def test():
with zipfile.ZipFile(test_zip) as zf:
for i in range(10):
zf.read(test_name)


if __name__ == "__main__":
import timeit
print(timeit.repeat("test()", setup="from __main__ import test", number=1, 
repeat=10))


On my machine (running a usual workload) the best of 10 was:

master:
3.812s

check closed:
3.874s

But I think my machine had a quite a bit of variance between runs.

--

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Thank you Daniel. But profiling is not good for benchmarking, it adds too much 
overhead. It can help to find narrow places, but for measuring effect of 
optimization you need to measure the raw time on non-debug build.

Semantically your changes are correct. I just want to know whether this adds a 
significant overhead. If it is, we should consider some kind of optimizations. 
It does not matter for Python implementation of BytesIO because in normal we 
use the C implementation.

--

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Daniel Hillier]

Daniel Hillier  added the comment:

Here's the script I used for profiling and the results I observed with and 
without the closed check in read:

import zipfile

test_zip = "time_test.zip"
test_name = "test_name.txt"

with zipfile.ZipFile(test_zip, "w") as zf:
zf.writestr(test_name, "Hi there! " * 300)

with zipfile.ZipFile(test_zip) as zf:
for i in range(10):
zf.read(test_name)

# Current code (no closed check), three different profiling sessions:
# ncalls  tottime  percall  cumtime  percall filename:lineno(function)
# 100.6120.0006.6380.000 zipfile.py:884(read)
# 100.5980.0006.4890.000 zipfile.py:884(read)
# 100.6000.0006.4850.000 zipfile.py:884(read)

# With closed check, three different profiling sessions:
# ncalls  tottime  percall  cumtime  percall filename:lineno(function)
# 100.6320.0006.5870.000 zipfile.py:884(read)
# 100.6230.0006.5640.000 zipfile.py:884(read)
# 100.6380.0006.7000.000 zipfile.py:884(read)

---

I based this change on the what BytesIO does: 
https://github.com/python/cpython/blob/master/Lib/_pyio.py#L912

Let me know if you want me to make any changes.

--

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Could you please test what performance effect it has on read()?

--
nosy: +serhiy.storchaka

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Daniel Hillier]

Change by Daniel Hillier :


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

___
Python tracker 

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

[issue37523] zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile

[Daniel Hillier]

New submission from Daniel Hillier :

After closing a file object opened from a ZipFile, attempting i/o operations 
raises AttributeError because the underlying fd has been set to None. We should 
be raising ValueErrors consistent with io.FileIO behaviour.

Similar inconsistencies exist for the following operations on a closed 
ZipExtFile:
- seek
- seekable
- read
- readable
- tell

--
components: Library (Lib)
messages: 347524
nosy: dhillier
priority: normal
severity: normal
status: open
title: zipfile: Raise ValueError for i/o operations on closed zipfile.ZipExtFile
type: behavior
versions: Python 3.7, Python 3.8

___
Python tracker 

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