Repository navigation
gh-81954: Always close ZipFile in __del__ even if the warning fails - #159419
brittanyrey wants to merge 3 commits into
Conversation
ZipFile.__del__ emitted its ResourceWarning before calling close(). When warnings.warn() raised, close() was skipped and the central directory of a writable archive was never written, leaving a corrupt file. This happens at interpreter shutdown, where resolving the lazy "warnings" import raises ImportError, and with -W error::ResourceWarning at any time. Call close() in a finally block.
|
Thanks for the review Sully @msullivan ! |
| buf = io.BytesIO() | ||
| zf = zipfile.ZipFile(buf, "w") | ||
| zf.writestr("f.txt", b"data") | ||
| with warnings.catch_warnings(), catch_unraisable_exception() as cm: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @requires_subprocess() | ||
| def test_unclosed_at_shutdown(self): | ||
| self.addCleanup(unlink, TESTFN) | ||
| script_helper.assert_python_ok("-c", f"""if 1: |
There was a problem hiding this comment.
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?
| @@ -0,0 +1,3 @@ | |||
| Fix data loss when a writable :class:`zipfile.ZipFile` that was never closed | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"
|
|
||
| @requires_subprocess() | ||
| def test_unclosed_at_shutdown(self): | ||
| self.addCleanup(unlink, TESTFN) |
There was a problem hiding this comment.
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
3 participants
BUG: Now that
ZipFiledeliversResourceWarningsduring__del__calls, if there was an issue lazily loadingwarningsduring finalization, theclose()call was missed entirely. This left the file in an unexpected state and introduced a possibility of data loss that didn't exist previously.FIX: Call close() in a finally block.