gh-148428: Fix file handle leak in pulldom.parse() by WYSIATI · Pull Request #148437 · python/cpython

WYSIATI · 2026-04-12T11:28:50Z

When pulldom.parse() is called with a filename string, it opens a file but the resulting DOMEventStream never closes it — no close(), no context manager, and clear() just sets self.stream = None without closing.

This adds resource management to DOMEventStream:

Track whether parse() opened the file via _owns_stream
Add close() that closes owned streams only
Add __enter__/__exit__ for with statement support
Add __del__ with ResourceWarning (follows the iterparse pattern from xml.etree.ElementTree.iterparse() should emit ResourceWarning when not explicitly closed #140601)
Update clear() to call close() before releasing references

The __del__ binds _warn=warnings.warn as a default argument to handle interpreter shutdown safely, matching the pattern in ElementTree.iterparse, subprocess.Popen, etc.

User-provided streams (passed as file objects to parse(), or via parseString()) are never closed by DOMEventStream.

Issue: xml.dom.pulldom.DOMEventStream leaks file handles opened by parse() #148428

📚 Documentation preview 📚: https://cpython-previews--148437.org.readthedocs.build/

When pulldom.parse() is called with a filename, the opened file handle is never closed. Add close(), context manager support, and __del__ with ResourceWarning to DOMEventStream. Update clear() to close owned streams.

Keep the existing test_parse unchanged; context manager behavior is covered by the new dedicated tests.

Remove test_context_manager_closes_file (covered by test_close_is_idempotent) and revert the unrelated modification to test_parse.

Use handler.close() instead of handler.stream.close() in addCleanup so the DOMEventStream is properly closed before GC triggers __del__.

github-actions · 2026-05-18T13:10:16Z

This PR is stale because it has been open for 30 days with no activity.

serhiy-storchaka

This is a good approach.

Note that __enter__ and __exit__ are not strictly needed -- contextlib.closing() works with any object having close(). They add convenience, but if there are more that one way to finish the object (clear() and close()), their semantic is ambiguous.

On other hand, why do you need close() if there is clear()? There are two variants of simplification:

Close the file in clear(), remove close(), and use clear() in __exit__().
Close the file in clear(), make close() an alias of clear(), remove __enter__ and __exit__.

The example in the documentation, which the issue quotes as the reason users hit this, is not updated.

The file object opened by parse() already emits its own ResourceWarning when collected unclosed, so DOMEventStream.__del__ is not needed. Rewrite test_context_manager_does_not_close_user_stream to actually use the context manager and pulldom.parse(), and drop comments that repeat the test names.

Per review: close() is removed, clear() now closes the stream when it was opened by parse() (and is safe to call more than once), and the context manager calls clear() on exit. Since minidom's _do_pulldom_parse() already calls clear(), this also fixes the file handle leak in xml.dom.minidom.parse() with a custom parser. Also update the leaking example in the documentation to use the with statement.

…leak-148428

WYSIATI · 2026-09-01T14:00:18Z

This is a good approach.

Note that __enter__ and __exit__ are not strictly needed -- contextlib.closing() works with any object having close(). They add convenience, but if there are more that one way to finish the object (clear() and close()), their semantic is ambiguous.

On other hand, why do you need close() if there is clear()? There are two variants of simplification:

Close the file in clear(), remove close(), and use clear() in __exit__().

Close the file in clear(), make close() an alias of clear(), remove __enter__ and __exit__.

The example in the documentation, which the issue quotes as the reason users hit this, is not updated.

The ambiguity is a valid point, I have cleaned up close() while keeping clear()

read-the-docs-community · 2026-09-01T14:01:19Z

Documentation build overview

📚 cpython-previews | 🛠️ Build #34344293 | 📁 Comparing 7112293 against main (374851f)

🔍 Preview build

4 files changed

± library/concurrent.interpreters.html
± library/xml.dom.pulldom.html
± library/xml.etree.elementtree.html
± whatsnew/changelog.html

serhiy-storchaka · 2026-09-01T16:56:38Z


   .. method:: DOMEventStream.reset()
+
+   .. method:: DOMEventStream.clear()


Oh, so it was not even documented?

Suggested change

.. method:: DOMEventStream.clear()

.. method:: clear()

This is already in the "class:: DOMEventStream" block.

I think that it would be better to fix the documentation of clear() and reset() in separate PR before merging this PR (it can be easily backported).

It looks like you referred to outdated code? Current implementation has method:: clear() without prefix.

Do you suggest submitting another PR just to fix the naming of DOMEventStream.reset()

serhiy-storchaka · 2026-09-01T17:02:06Z

@@ -275,8 +282,11 @@ def _emit(self):

    def clear(self):
        """clear(): Explicitly release parsing objects"""


"clear():" is redundant. The docstring still doesn't mention closing.

The latest code has such wording:

"""Release the parsing objects. The stream is closed if it was opened by parse(). """

Do you think this is not appropriate?

serhiy-storchaka

Also, is there a test for clear() with user stream?

…lear() test

WYSIATI · 2026-09-02T01:29:35Z

Also, is there a test for clear() with user stream?

Added test_clear_does_not_close_user_stream.

…leak-148428

serhiy-storchaka · 2026-09-02T16:57:08Z

I just fixed the clear() documentation together with other documentation (see #156839). Please merge main into your branch and resolve conflicts.

pythongh-148428: Fix file handle leak in pulldom.parse()

12efd8a

When pulldom.parse() is called with a filename, the opened file handle is never closed. Add close(), context manager support, and __del__ with ResourceWarning to DOMEventStream. Update clear() to close owned streams.

bedevere-app Bot added the awaiting review label Apr 12, 2026

bedevere-app Bot mentioned this pull request Apr 12, 2026

xml.dom.pulldom.DOMEventStream leaks file handles opened by parse() #148428

Open

WYSIATI added 5 commits April 12, 2026 20:59

pythongh-148428: Revert off-topic change to test_parse

282886b

Keep the existing test_parse unchanged; context manager behavior is covered by the new dedicated tests.

pythongh-148428: Remove redundant test and off-topic change

57840f6

Remove test_context_manager_closes_file (covered by test_close_is_idempotent) and revert the unrelated modification to test_parse.

pythongh-148428: Fix test_parse ResourceWarning from __del__

baaf87f

Use handler.close() instead of handler.stream.close() in addCleanup so the DOMEventStream is properly closed before GC triggers __del__.

Merge branch 'main' into fix-pulldom-resource-leak-148428

96ca1f9

Merge branch 'main' into fix-pulldom-resource-leak-148428

1e52337

github-actions Bot added the stale Stale PR or inactive for long period of time. label May 18, 2026

serhiy-storchaka reviewed Aug 30, 2026

View reviewed changes

Comment thread Lib/test/test_pulldom.py Outdated

Comment thread Lib/test/test_pulldom.py Outdated

Comment thread Lib/xml/dom/pulldom.py Outdated

WYSIATI added 3 commits September 1, 2026 21:45

Merge remote-tracking branch 'origin/main' into fix-pulldom-resource-…

a28581c

…leak-148428

serhiy-storchaka reviewed Sep 1, 2026

View reviewed changes

pythongh-148428: Address review: docstring, doc markup, user-stream c…

9984c8f

…lear() test

Merge remote-tracking branch 'origin/main' into fix-pulldom-resource-…

7112293

…leak-148428

Navigation Menu

Uh oh!

Uh oh!

gh-148428: Fix file handle leak in pulldom.parse() - #148437

gh-148428: Fix file handle leak in pulldom.parse()#148437
WYSIATI wants to merge 11 commits into
python:mainfrom
WYSIATI:fix-pulldom-resource-leak-148428

WYSIATI commented Apr 12, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

serhiy-storchaka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WYSIATI commented Sep 1, 2026

Uh oh!

read-the-docs-community Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

serhiy-storchaka Sep 1, 2026

Uh oh!

WYSIATI Sep 2, 2026

Uh oh!

serhiy-storchaka Sep 1, 2026

Uh oh!

WYSIATI Sep 2, 2026

Uh oh!

serhiy-storchaka left a comment

Uh oh!

WYSIATI commented Sep 2, 2026

Uh oh!

serhiy-storchaka commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		.. method:: DOMEventStream.reset()

		.. method:: DOMEventStream.clear()

		@@ -275,8 +282,11 @@ def _emit(self):

		def clear(self):
		"""clear(): Explicitly release parsing objects"""

Uh oh!

Uh oh!

Conversation

WYSIATI commented Apr 12, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WYSIATI commented Sep 1, 2026

Uh oh!

read-the-docs-community Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

serhiy-storchaka Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

WYSIATI Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

WYSIATI Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

WYSIATI commented Sep 2, 2026

Uh oh!

serhiy-storchaka commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WYSIATI commented Apr 12, 2026 •

edited by github-actions Bot

Loading

read-the-docs-community Bot commented Sep 1, 2026 •

edited

Loading