Skip to content

gh-81954: Eagerly import warnings module to failure of ZipFile.__del__ during finalization #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 4 commits into
base: main
Choose a base branch
from
+14 −2
Open

gh-81954: Eagerly import warnings module to failure of ZipFile.__del__ during finalization #159419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 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
48693fb
Import warnings eagerly in zipfile and simplify the tests
brittanyrey Oct 15, 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
10 changes: 9 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,14 @@ def test_garbage_collection(self):
zf.close()
del zf

# ResourceWarning escalated to an error is reported as unraisable.
zf = zipfile.ZipFile(io.BytesIO(), "w")
with (warnings.catch_warnings(action="error", category=ResourceWarning),
catch_unraisable_exception() as cm):
del zf
gc_collect()
self.assertIs(cm.unraisable.exc_type, ResourceWarning)
Comment on lines +4098 to +4104

@vstinner vstinner Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh. You simplified the test too much, it doesn't test that the zipfile was written properly. For example, it doesn't catch a now obvious error: close() is not called if ResourceWarning is treated as an error (raise an exception). I suggest to rewrite the test as:

Suggested change
# ResourceWarning escalated to an error is reported as unraisable.
zf = zipfile.ZipFile(io.BytesIO(), "w")
with (warnings.catch_warnings(action="error", category=ResourceWarning),
catch_unraisable_exception() as cm):
del zf
gc_collect()
self.assertIs(cm.unraisable.exc_type, ResourceWarning)
# ResourceWarning escalated to an error is reported as unraisable.
buf = io.BytesIO()
zf = zipfile.ZipFile(buf, "w")
zf.writestr("f.txt", b"data")
with (warnings.catch_warnings(action="error", category=ResourceWarning),
catch_unraisable_exception() as cm):
del zf
gc_collect()
self.assertIs(cm.unraisable.exc_type, ResourceWarning)
with zipfile.ZipFile(buf, "r") as zf:
self.assertEqual(zf.namelist(), ["f.txt"])
self.assertEqual(zf.read("f.txt"), b"data")


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
2 changes: 1 addition & 1 deletion Lib/zipfile/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import sys
import threading
import time
lazy import warnings
import warnings # must stay eager to support finalization

@vstinner vstinner Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You reverted __del__() change which reintroduced the bug that you're supposed to fix.

I suggest using the same code pattern than subprocess.Popen.__del__() to keep a reference to warnings.warn until the destructor is actually called:

    def __del__(self, _warn=warnings.warn):
        """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.
        try:
            if self.fp is not None and self.mode in ('w', 'x', 'a'):
                _warn(f"unclosed ZipFile {self!r}",
                      ResourceWarning, source=self, stacklevel=2)
        finally:
            self.close()


try:
import zlib # We may need its compression method
Expand Down
4 changes: 4 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,4 @@
:mod:`zipfile` now imports :mod:`warnings` eagerly so that a writable
:class:`~zipfile.ZipFile` left unclosed at interpreter shutdown can emit its
:exc:`ResourceWarning` and be closed instead of failing with an
:exc:`ImportError`.

@vstinner vstinner Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should revert your __del__() code which made sure that close() is called, and so you should use your previous Changelog entry.

Loading