GeezerVison

Nice work. +1 on a backport as I think this can result in data loss if there's data waiting to be flushed from the buffer or compressor.

Should we add some docs saying that the GzipFile object should be closed explicitly instead of relying on finalizers to clean it up so as to not rely on GC heuristics?

@danifus 👍. The report production code did a .close() it sounds like (#129726 (comment)), feels to me like another issue that could be resolved: Close to me should mean all data is flushed and the GzipFile will not try to write again (and its write methods should error). Not sure what this last little bit of data is / why it was around after .close() and tried getting written on __del__.

Hmmm I had missed that production example and it baffles me :p I don't see how more data is attempted to be written after .close() has been called once.

We have this check at the start of .close():

and this in the finally block at the end of that .close():

Seems pretty watertight.

@xrmx it's possible you saw this issue in your code in the logger.error("dropping flushed data due to transport failure back-off") branch of the following if statement (the one that doesn't close the GzipFile). Let me know if that's not the case and I'll keep investigating.

        if not self.state.should_try():
            logger.error("dropping flushed data due to transport failure back-off")
        else:
            fileobj = buffer.fileobj  # get a reference to the fileobj before closing the gzip file
            buffer.close()

            data = fileobj.getbuffer()
            data = gzip.compress(buffer.getvalue(), compresslevel=self._compress_level)

@xrmx it's possible you saw this issue in your code in the logger.error("dropping flushed data due to transport failure back-off") branch of the following if statement (the one that doesn't close the GzipFile). Let me know if that's not the case and I'll keep investigating.

        if not self.state.should_try():
            logger.error("dropping flushed data due to transport failure back-off")
        else:
            fileobj = buffer.fileobj  # get a reference to the fileobj before closing the gzip file
            buffer.close()

            data = fileobj.getbuffer()
            data = gzip.compress(buffer.getvalue(), compresslevel=self._compress_level)

I think I've only seen it from buffer.close(). Still with the patched 3.13 (so +2 on line numbers) I have seen (at least) another occurrence of the same stacktrace:

Traceback (most recent call last):
  File "/usr/lib/python3.13/gzip.py", line 361, in close
    fileobj.write(self.compress.flush())
ValueError: I/O operation on closed file.

It comes from test_send_remote_failover_sync_non_transport_exception_error that on its own prints these:

Exception ignored in: <_io.BytesIO object at 0x7f54eae0a6b0>
BufferError: Existing exports of data: object cannot be re-sized
Exception ignored in: <_io.BytesIO object at 0x7f54eada7420>
BufferError: Existing exports of data: object cannot be re-size

And when run with another test sometimes prints the traceback. I also see these BufferError whenhandle_transport_fail is called. But I'll need some time to understand what's going on and provide another reproducer.

That stacktrace could appear in 2 ways:

• calling .close() directly
• .close() being called in the finalizer before being garbage collected (__del__()).

Both would give you that traceback location but only the occurrence in the finalizer as the object goes out of scope will give you the Exception ignored in: <gzip on 0x7fa4fd99c550> text as well (exceptions are ignored in finalizers).

BufferError: Existing exports of data: object cannot be re-size is kind of related to this. You can't write to or close a BytesIO object if the buffer returned from BytesIO.getbuffer() is still held somewhere.

If you claim the buffer with BytesIO.getbuffer() before closing the GzipFile, when the GzipFile is closed (or destroyed / finalized as it goes out of scope), data will attempt to be flushed to the BytesIO object in .close() and you will get the BufferError: Existing exports of data: object cannot be re-size exception. Try doing something like:

gzf = GzipFile(fileobj=io.BytesIO())
fileobj = gzf.fileobj
gzf.close()
buf = fileobj.getbuffer()  # Grab the buffer after the GzipFile obj has been closed

As for the original issue, are you able to see if code like this:

        if not self.state.should_try():
            buffer.close()  # close buffer in this branch of the if statement too
            logger.error("dropping flushed data due to transport failure back-off")
        else:
            ...

resolves your issue without this weak reference fix applied to your python?

That stacktrace could appear in 2 ways:

* calling `.close()` directly

* `.close()` being called in the finalizer before being garbage collected (`__del__()`).

Both would give you that traceback location but only the occurrence in the finalizer as the object goes out of scope will give you the Exception ignored in: <gzip on 0x7fa4fd99c550> text as well (exceptions are ignored in finalizers).

BufferError: Existing exports of data: object cannot be re-size is kind of related to this. You can't write to or close a BytesIO object if the buffer returned from BytesIO.getbuffer() is still held somewhere.

If you claim the buffer with BytesIO.getbuffer() before closing the GzipFile, when the GzipFile is closed (or destroyed / finalized as it goes out of scope), data will attempt to be flushed to the BytesIO object in .close() and you will get the BufferError: Existing exports of data: object cannot be re-size exception. Try doing something like:

gzf = GzipFile(fileobj=io.BytesIO())
fileobj = gzf.fileobj
gzf.close()
buf = fileobj.getbuffer()  # Grab the buffer after the GzipFile obj has been closed

Thanks for the explanation, adding the explicit buffer.close() on the other path does not remove the Exception ignored in: <gzip on 0x7f551ebd2c80> I'm still seeing with the patch applied. I need to dig more.

As for the original issue, are you able to see if code like this:

        if not self.state.should_try():
            buffer.close()  # close buffer in this branch of the if statement too
            logger.error("dropping flushed data due to transport failure back-off")
        else:
            ...

resolves your issue without this weak reference fix applied to your python?

Nope, adding buffer.close() does not remove all the gzip stacktraces I was seeing before.

Oh the elephant in the room is that this code is running as a daemon thread and so it well may be that I got these when it gets killed at exit.

Oh the elephant in the room is that this code is running as a daemon thread and so it well may be that I got these when it gets killed at exit.

That's not the problem, after poking a bit with the code the following quote from @danifus started to make sense to me:

If you claim the buffer with BytesIO.getbuffer() before closing the GzipFile, when the GzipFile is closed (or destroyed / finalized
as it goes out of scope), data will attempt to be flushed to the BytesIO object in .close() and you will get the BufferError: Existing
exports of data: object cannot be re-size exception.

Adding a data.release() after closing the gzip appears to have fixed the other warnings. Now I still have a single occurrence of the original stacktrace but it's near a test that now raises ValueError: operation forbidden on released memoryview object because of the memoryview release, so maybe it's poking with internals too.

BTW the BytesIO doc talks about a view that you should release but it doesn't say how and it doesn't link the memoryview doc.

Thanks a lot @cmaloney and @danifus for the help!

Glad to hear most of the exceptions are gone.

As for the last one, I think you'll just want to del the instance of the view you got from .getbuffer(). I think your current issue is that there are 2 references to the same memoryview and your .release() in one location is impacting the other.

Instead, just do something like:

bio = BytesIO()
buf = bio.getbuffer()
del buf  # remove this instance and let the GC release it if no other references exist.

Glad to hear most of the exceptions are gone.

As for the last one, I think you'll just want to del the instance of the view you got from .getbuffer(). I think your current issue is that there are 2 references to the same memoryview and your .release() in one location is impacting the other.

Instead, just do something like:

bio = BytesIO()
buf = bio.getbuffer()
del buf  # remove this instance and let the GC release it if no other references exist.

The last one is from a test mocking the function getting the view as arg and trying to read it after it has already been released. If I change the code as you suggested running this test I get EOFError: Compressed file ended before the end-of-stream marker was reached from /usr/lib/python3.13/gzip.py:616: EOFError.

Ah ok, that does sound a bit more involved. You're right, the .release() method is the right way to go.

As far as this issue is concerned, I think the .close() method doesn't have any issues and we should add some documentation saying that GzipFile should be closed explicitly to avoid a potential race condition during garbage collection (mitigated with this patch but may be subject to resurrection if the GC algorithm is changed or a different python implementation uses a different GC). What do you guys think?

I think it's still worthwhile to fix, since CPython sholdn't error if a GzipFile doesn't explicitly have .close() called. That's likely if an exception is thrown and code isn't using with / a context manager.

Definitely some Documentation improvements would also be good I think. .close() is definitely important for getting correct behavior.

As far as this issue is concerned, I think the .close() method doesn't have any issues and we should add some documentation saying that GzipFile should be closed explicitly to avoid a potential race condition during garbage collection (mitigated with this patch but may be subject to resurrection if the GC algorithm is changed or a different python implementation uses a different GC). What do you guys think?

Fyi without the gzip patch and with the explicit cleanup in my code I still see a ton of ValueError: I/O operation on closed file. warnings and it's not trivial to track them down 😓

@vstinner I think this is ready for review. (Not sure why the issue-number bot didn't link it back to the original issue). Exception on close issue that became visible with your change to log exceptions if close() fails (GH-12786 which was part of https://bugs.python.org/issue18748 / gh-62948)

Thanks @cmaloney for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

GH-130669 is a backport of this pull request to the 3.13 branch.

Thanks @cmaloney for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

GH-130670 is a backport of this pull request to the 3.12 branch.

loop found by @danifus , bug found and explored by @xrmx

PR merged, thank you all! I backported the fix to 3.12 and 3.13.