Repository navigation
ResourceWarning in GzipFile (write mode) if constructor raises (3.14 only) #131461
Description
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
Activity
pythongh-131461: fix ResourceWarning when writing a unwritable gzipfile
[-]ResourceWarning in GzipFile (write mode) if constructor raises[/-][+]ResourceWarning in GzipFile (write mode) if constructor raises (3.14 only)[/+]cmaloney commentedon Mar 19, 2025
Want to separate out a couple pieces for clarity here, new to this code example, want to validate my reading of it:
GzipFileis partially constructed currently and warns on destruction about not being closed.GzipFile.tarfile_testdoesn't refer to GzipFile code directly, produces the warning but shouldn'tI 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 commentedon Mar 19, 2025
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
withcmaloney commentedon Mar 19, 2025
Agreed, 1 should mean you don't get a warning in the sample code. It's important for
GzipFileto be well behaved when it encounters exceptions on construction just like general Python code should be.cmaloney commentedon Mar 19, 2025
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
GzipFileit is better to be robust, that is part of why 3 is a distinct problem for me.tarfileshould be well behaved; that it defers to aGzipFileinternally 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 thetarfilecode as something that could be strengthened.gh-131492, gh-131461: handle exceptions in GzipFile constructor while…
pythongh-131492, pythongh-131461: handle exceptions in GzipFile const…
pythongh-131492, pythongh-131461: handle exceptions in GzipFile const…
[3.13] gh-131492, gh-131461: handle exceptions in GzipFile constructo…
[3.12] gh-131492, gh-131461: handle exceptions in GzipFile constructo…
vstinner commentedon Mar 21, 2025
Fixed by the change ce79274.
vstinner commentedon Mar 21, 2025
Oh, I still get a ResourceWarning when I run the reproducer script:
I reopen the issue.
graingert commentedon Mar 21, 2025
apologies, the demo code was slightly off, it should have been
if not self.first::vstinner commentedon Mar 21, 2025
Aha, with the adjusted reproducer, I don't get a ResourceWarning anymore. I close again the issue :-)