ResourceWarning in GzipFile (write mode) if constructor raises (3.14 only)

[graingert]

Bug report

Bug description:

import io
import gzip

class BadFile(io.BytesIO):
    first = False
    def write(self, data):
        if self.first:
            self.first = False
            raise OSError

def main():
    try:
        gzip.GzipFile(fileobj=BadFile(), mode="w")
    except OSError:
        pass

main()

when run produces:

./python -W error ../../demo.py
Exception ignored while calling deallocator <function GzipFile.__del__ at 0x7b14defe09e0>:
Traceback (most recent call last):
  File "/home/graingert/projects/cpython/Lib/gzip.py", line 458, in __del__
    warnings.warn("unclosed GzipFile",
ResourceWarning: unclosed GzipFile

This warning is also raised by the test suite, in test_tarfile.WriteTestBase.test_open_nonwritable_fileobj

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-131492: gh-131461: handle exceptions in GzipFile constructor while owning resources #131462

[cmaloney]

Want to separate out a couple pieces for clarity here, new to this code example, want to validate my reading of it:

1. A GzipFile is partially constructed currently and warns on destruction about not being closed.
2. The code sample doesn't close the GzipFile.
3. (pulling in from PR) tarfile_test doesn't refer to GzipFile code directly, produces the warning but shouldn't

I think all three of these should definitely be improved, just that they're somewhat distinct. 1 and 3 should definitely be improved in CPython itself. Looks like your PR helps primarily with 1.

This warning was added in 3.14 after code inspection around user reports in Python 3.12 GzipFile (gh-129726, gh-129640), showed there could be data loss if not properly closed. There was a reference loop in 3.12 (now with backported fix) that was resulting in an unexpected close order if the GzipFile wasn't explicitly closed when passed temporary files.

For 2, I think the sample code would be more robust by using with / Context Managers and possibly .detach() (if it doesn't want to actually close the underlying). Full details of different ways to resolve the warning by making code robust are in in gh-130806.

[graingert]

for 2 you shouldn't need to close a GzipFile if it raises in the constructor, we use self.assertRaises so we know the tarfile.open is responsible for closing its resources before raising, so we don't need to use with

[cmaloney]

Agreed, 1 should mean you don't get a warning in the sample code. It's important for GzipFile to be well behaved when it encounters exceptions on construction just like general Python code should be.

[cmaloney]

Tests / samples can (and often do) reduce code to increase clarity and make sure that catch errors expect. As a test case, I think the sample code is reasonable. In general code which uses GzipFile it is better to be robust, that is part of why 3 is a distinct problem for me. tarfile should be well behaved; that it defers to a GzipFile internally should be largely an implementation detail. Fixing 1 I suspect will make the error for 3 go away, but 3 tells me to double check the tarfile code as something that could be strengthened.

[vstinner]

Fixed by the change ce79274.

[vstinner]

Oh, I still get a ResourceWarning when I run the reproducer script:

/home/vstinner/python/main/demo.py:13: ResourceWarning: unclosed GzipFile
  gzip.GzipFile(fileobj=BadFile(), mode="w")
ResourceWarning: Enable tracemalloc to get the object allocation traceback

I reopen the issue.

[graingert]

apologies, the demo code was slightly off, it should have been if not self.first::

import io
import gzip

class BadFile(io.BytesIO):
    first = False

    def write(self, data):
        if not self.first:
            self.first = False
            raise OSError

def main():
    try:
        gzip.GzipFile(fileobj=BadFile(), mode="w")
    except OSError:
        pass
    else:
        assert False, "did not raise OSError!"

main()

[vstinner]

Aha, with the adjusted reproducer, I don't get a ResourceWarning anymore. I close again the issue :-)