gh-140601: Add ResourceWarning to iterparse when not closed

[osamakader]

When iterparse() opens a file by filename and is not explicitly closed, emit a ResourceWarning to alert developers of the resource leak.

This implements the TODO comment at line 1270 of ElementTree.py which has been requesting this feature since the close() method was added.

• Add _closed flag to IterParseIterator to track state
• Emit ResourceWarning in del if not closed
• Add comprehensive test cases
• Update existing tests to properly close iterators

• Issue: xml.etree.ElementTree.iterparse() should emit ResourceWarning when not explicitly closed #140601

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[cmaloney]

Please don't force push to CPython PRs; it's preferred to keep commit history intact for easier review. See: https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide

[cmaloney]

cc: @serhiy-storchaka who added the close() ResourceWarning comment

[ashm-dev]

Wouldn't it be better to use support.gc_collect() instead of directly calling gc.collect() was specifically added for use in tests to ensure consistent and deterministic garbage collection behavior across platforms and configurations.

[serhiy-storchaka]

These tests were intentionally added to check that the resource is closed without explicit close(). Don't add close(). Just replace check_no_resource_warning() with assertWarns(ResourceWarning) and add gc_collect(). There are other tests that explicitly call close().

[osamakader]

Hi @serhiy-storchaka

I've addressed all the review feedback:
All tests pass locally. However, CI shows "Unraisable exception"
warnings in the C extension tests (test_xml_etree_c). The ResourceWarning
is still being emitted correctly, but it's reported as "unraisable".

Is this expected behavior for warnings in __del__ with C extensions,
or do I need to adjust the implementation?

Thanks!

[serhiy-storchaka]

No, it is not an expected behavior.

I have fixed the warnings. The underlying file is closed when the iterator has been exhausted or raised an exception, so no explicit close() is needed in this case. And many tests depended on this. I have removed a ResourceWarning in such case.

Please add a versionchanged directive in the module documentation. I am not sure about the What's New document. This is a minor change, so it may not need a What's New entry.

[osamakader]

Thanks @serhiy-storchaka for the support.

[serhiy-storchaka]

I did not merge this PR yet, because there is an important question -- should the warning be emitted if the generator was automatically closed after iterating to the end (or after parsing error)? I silenced them, otherwise many existing tests would need an explicit close().

But now I think that the warning should be emitted even if the file was closed. Yes, this will force users to add an explicit close() after iteration (or use with closing(iterparse(...))) in the working code, but this will save them in exceptional cases -- when the iteration was interrupted and never finished.

[cmaloney]

👍 to always emitting the warning. I think more critical in potential data loss cases (ex. there's a write buffer) but it's also worthwhile when it helps prevent hard to track down easy to resolve bugs

[ashm-dev]

Maybe this is better?

Suggested change

	try:
	_warn(f"unclosed iterparse iterator {source.name!r}", ResourceWarning, stacklevel=2)
	finally:
	source.close()
	name = getattr(source, 'name', None)
	if name:
	_warn("unclosed iterparse iterator %r" % (name,),
	ResourceWarning, stacklevel=2)
	source.close()

[serhiy-storchaka]

source.name always exist and not empty. This code is only executed when iterparse() opened a file by name.

BTW, if the iterparse() argument was file descriptor 0, your variant would not work.

[ashm-dev]

You're right - I missed that close_source=True guarantees source.name exists, and if name: breaks on fd 0. Thanks for catching that!

The original code is correct. (Though % formatting might be slightly safer than f-string in __del__, but not a blocker.)

[ashm-dev]

Wouldn't this option be better here?

try:
  if close_source:
    close_source = False
    source.close()
finally:
  gen.close()

[serhiy-storchaka]

I wonder. It seems that only exception can be raised here is a KeyboardInterrupt. The difference between calling and not calling gen.close() is whether you can continue iterating after iterparse().close() (cached elements can still be yielded). But if iterparse().close() failed, you do not have any guarantees. Anyway, the file is closed first, so there will not be leaks. I think there will be no practical difference, so we can keep the simplest code.

[ashm-dev]

Wouldn't it be better to split one huge test into several smaller ones, since this test has already become too big?

[serhiy-storchaka]

Maybe. There are 4 different cases here.

[serhiy-storchaka]

There are other big tests here, so it is worth to refactor them before merging this PR. See #141499.

[ashm-dev]

I agree