Skip to content

gh-81954: Always close ZipFile in __del__ even if the warning fails #159419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
brittanyrey wants to merge 3 commits into
base: main
Choose a base branch
from
+35 −5
Open

gh-81954: Always close ZipFile in __del__ even if the warning fails #159419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e48aea1
Always close ZipFile in __del__ even if the warning fails
brittanyrey Oct 5, 2026
29ef754
Add NEWS entry
brittanyrey Oct 14, 2026
9c41b95
Explain why ZipFile.__del__ wraps the warning in try/finally
brittanyrey Oct 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion Lib/test/test_zipfile/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from test.support import (
findfile, requires_zlib, requires_bz2, requires_lzma,
requires_zstd, captured_stdout, captured_stderr, requires_subprocess,
cpython_only, gc_collect
cpython_only, gc_collect, catch_unraisable_exception
)
from test.support.os_helper import (
TESTFN, unlink, rmtree, temp_dir, temp_cwd, fd_count, FakePath,
Expand Down Expand Up @@ -4095,6 +4095,29 @@ def test_garbage_collection(self):
zf.close()
del zf

def test_garbage_collection_closes_when_warning_fails(self):
buf = io.BytesIO()
zf = zipfile.ZipFile(buf, "w")
zf.writestr("f.txt", b"data")
with warnings.catch_warnings(), catch_unraisable_exception() as cm:

@cmaloney cmaloney Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified a bit to:

with (warnings.catch_warnings(action="error", category=ResourceWarning),
      catch_unraisable_exception() as cm):
    del zf
    gc_collect()
    self.assertIs(cm.unraisable.exc_type, ResourceWarning)

Another option is making it so we just test with the test_unclosed_at_shutdown style passing -W to the interpreter which would reduce code a bit more while covering the same case.

@cmaloney cmaloney Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be more cases in the existing test_garbage_collection rather than a new test section. It's adding a new case we want to check

warnings.simplefilter("error", ResourceWarning)
del zf
gc_collect()
self.assertIsInstance(cm.unraisable.exc_value, ResourceWarning)
with zipfile.ZipFile(buf) as zf:
self.assertEqual(zf.namelist(), ["f.txt"])

@requires_subprocess()
def test_unclosed_at_shutdown(self):
self.addCleanup(unlink, TESTFN)

@cmaloney cmaloney Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this particular test suite already cleans up TESTFN so I don't think this needs to be done here:

cpython/Lib/test/test_zipfile/test_core.py

Lines 487 to 489 in a6b4aa6

def tearDown(self):
unlink(TESTFN)
unlink(TESTFN2)

script_helper.assert_python_ok("-c", f"""if 1:

@cmaloney cmaloney Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still ends up raising an ImportError for zipfile which gets printed to stderr. Is there a way we could make the fix so in this unclosed at interpreter shutdown case we import warnings before interpreter shutdown?

import zipfile
zf = zipfile.ZipFile({TESTFN!r}, "w")
zf.writestr("f.txt", b"data")
""")
with zipfile.ZipFile(TESTFN) as zf:
self.assertEqual(zf.namelist(), ["f.txt"])

def test_unsupported_version(self):
# File has an extract_version of 120
data = (b'PK\x03\x04x\x00\x00\x00\x00\x00!p\xa1@\x00\x00\x00\x00\x00\x00'
Expand Down
12 changes: 8 additions & 4 deletions Lib/zipfile/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2650,10 +2650,14 @@ def __del__(self):
"""Call the "close()" method in case the user forgot."""
# gh-81954: Warn if writable ZipFile is implicitly closed.
# GC cleanup order is non-deterministic and can result in data loss.
if self.fp is not None and self.mode in ('w', 'x', 'a'):
warnings.warn(f"unclosed ZipFile {self!r}",
ResourceWarning, source=self, stacklevel=2)
self.close()
try:
# Wrap within a try to ensure the file is closed even if the
# warnings module is unable to load during finalization.
if self.fp is not None and self.mode in ('w', 'x', 'a'):
warnings.warn(f"unclosed ZipFile {self!r}",
ResourceWarning, source=self, stacklevel=2)
Comment thread
brittanyrey marked this conversation as resolved.
finally:
self.close()

def close(self):
"""Close the file, and for mode 'w', 'x' and 'a' write the ending
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-10-14-14-47-05.gh-issue-81954.KvM0gY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix data loss when a writable :class:`zipfile.ZipFile` that was never closed

@cmaloney cmaloney Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user didn't close the file either, this doesn't make a guarantee the zipfile is closed cleanly (GC order is not guaranteed). Definitely improves an edge case when warnings is not importable because the interpreter is shutting down. It would be nice if we could make that warnings reification happen before shutdown. Maybe using _warnings directly in this case? (that is always loaded I think)

@cmaloney cmaloney Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking a bit more here: I think we should just make the warnings import non-lazy: Being lazy with usage only in __del__ leads to this edge case, and the small import time increase is not a bad tradeoff for "we emit warnings properly"

is finalized during interpreter shutdown: :meth:`~zipfile.ZipFile.close` is
now called even if emitting the :exc:`ResourceWarning` fails.
Loading