Skip to content

bpo-30264: xml.sax.parse() closes the parser on error#1444

Closed
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:sax_res_warn
Closed

bpo-30264: xml.sax.parse() closes the parser on error#1444
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:sax_res_warn

Conversation

@vstinner

@vstinner vstinner commented May 3, 2017

Copy link
Copy Markdown
Member

The xml.sax.parse() function now closes the parser on error to not leak resources like open files.

@vstinner vstinner added the type-bug An unexpected behavior, bug, or error label May 3, 2017
@mention-bot

Copy link
Copy Markdown

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bitdancer, @serhiy-storchaka and @Yhg1s to be potential reviewers.

The xml.sax.parse() function now closes the parser on error to not
leak resources like open files.

@louisom louisom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a Misc/NEWS entry. This change changes behavior. It can change visible exception.

Comment thread Lib/test/test_sax.py
self.check_parse(TESTFN)
with open(TESTFN, 'rb') as f:
with self.assertRaises(SAXException):
self.check_parse(f)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't check that the file is closed now?

@vstinner

vstinner commented May 4, 2017

Copy link
Copy Markdown
Member Author

I proposed a different approach: see PR #1451.

@vstinner

vstinner commented May 4, 2017

Copy link
Copy Markdown
Member Author

Abandonned in favor of the PR #1451.

@vstinner vstinner closed this May 4, 2017
@vstinner vstinner deleted the sax_res_warn branch May 4, 2017 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants