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

[graingert]

• Issue: ResourceWarning in GzipFile (write mode) if constructor raises (3.14 only) #131461

• Issue: GzipFile leaves GzipFile.myfileobject open in constructor if exception is raised #131492

[vstinner]

@cmaloney: Would you mind to review this fix?

[vstinner]

Would it be possible to write a test with such broken file object?

[graingert]

Would it be possible to write a test with such broken file object?

There's already a test in test_tarfile: test_open_nonwritable_fileobj the failure will be enforced by #128973

I added check_no_resource_warning to make the assert hold until then

[vstinner]

LGTM

[vstinner]

The CI fails:

TypeError: check_no_resource_warning() missing 1 required positional argument: 'testcase'

[graingert]

The CI fails:

TypeError: check_no_resource_warning() missing 1 required positional argument: 'testcase'

🤦

[graingert]

I pushed a more thorough change inspired by

cpython/Lib/ssl.py

Lines 1014 to 1015 in a4832f6

	# Now SSLSocket is responsible for closing the file descriptor.
	try:

previously it was possible for this self.myfileobj to be left open if any of the constructor failed

[cmaloney]

Rather than the try/except wrapping, why not add a flag which is set to False at the beginning of the constructor and True at the end, in __del__ (which is what emits the resource warning), check that flag. If it's not fully constructed then don't emit the resource warning?

The I/O stack (GzipFile inherits from IOBase) in IOBase.__del__ always does the close call if self.closed is False, so shouldn't need to manually add a special case here. GzipFile.closed is:

    @property
    def closed(self):
        return self.fileobj is None

(so if there is a fileobj set, close should always be called).

[graingert]

This wouldn't help with myfileobj closing

[cmaloney]

How about using self.fileobj is not None as the "fully initialized" flag (already set to None always, only set to non-None at end of __init__), and checking around if self.filobj is None and self.myfileobj is not None in __del__?

It feels a lot like this is adding more cases / code around closing. Trying to find a way to get this change to be smaller / more minimal to reduce risks. IO stack construction, dealloc warnings, and close + destruction order is already really intricate and hard to understand in full.

[graingert]

We can't rely on self.myfileobj being closed in __del__ and we don't return an instance of GzipFile for the caller to close so self.myfileobj needs to be closed at the end of __init__ if there's an exception: so we need the try/catch

[graingert]

Right but this is closing objects that are inaccessible to the caller

[cmaloney]

The GzipFile / self is equally inaccessible, and cleaned up by its __del__ / finalizer tp_finalize (or tp_dealloc). It's guaranteed __del__ is called and able to release any held resources (such as self.myfileobj) see PEP-442.

I'm okay adding a cleanup for the case where self.fileobj is None and self.myfileobj is not None inside __del__. Matches the behavior of other io objects and is a lot smaller diff.

[graingert]

This is not correct, you cannot rely on __del__ to be called when an exception is raised because the exception traceback holds a reference to self

[cmaloney]

That statement seems to contradict PEP 442 Predictability section to me...

Following this scheme, an object’s finalizer is always called exactly once, even if it was resurrected afterwards.
For CI objects, the order in which finalizers are called (step 2 above) is undefined.

Even if there is an exception traceback that keeps the reference to self, eventually that traceback will go out of scope and file will be closed / not leaked...

[cmaloney]

I'll defer to @vstinner and @serhiy-storchaka for approval. From my perspective, this is a lot more complex of a bugfix than required, didn't match stated description and arguments around self.myfileobj. Happy to do quick fixes, can produce a much smaller patch here if desired with less behavior change.

[serhiy-storchaka]

LGTM.

I would add support.gc_collect() in tests.

[vstinner]

LGTM

[miss-islington-app]

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

[bedevere-app]

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

[bedevere-app]

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

[vstinner]

Hum, the backports mention ResourceWarning, whereas ResourceWarning only exists in Python 3.14. If we want to backport this change to 3.13 and 3.12, maybe references to ResourceWarning should be removed?

What do you think @graingert?

[graingert]

Oh yeah nix the 3.14 specific bits of the changelog from the backports

[graingert]

I've removed the references to a ResourceWarning in the backports