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

[brittanyrey]

BUG: Now that ZipFile delivers ResourceWarnings during __del__ calls, if there was an issue lazily loading warnings during finalization, the close() 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: Eager import warnings

• Issue: ValueError: I/O operation on closed file. in ZipFile destructor #81954

[msullivan]

Seems good to me

[brittanyrey]

Thanks for the review Sully @msullivan !

[brittanyrey]

Updated!

[cmaloney]

The NEWS is a little bit long but overall looking good. I like the comment about why the import warnings is not lazy.

[vstinner]

You reintroduced a bug when you simplified your change.

[vstinner]

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")

[vstinner]

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()

[vstinner]

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

further review