Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented May 3, 2017

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

@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.
Copy link
Contributor

@louisom louisom left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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.

# Collect leaked file.
gc.collect()
with self.assertRaises(SAXException):
self.check_parse(TESTFN)
with open(TESTFN, 'rb') as f:
with self.assertRaises(SAXException):
self.check_parse(f)
Copy link
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
Copy link
Member Author

vstinner commented May 4, 2017

I proposed a different approach: see PR #1451.

@vstinner
Copy link
Member Author

vstinner commented May 4, 2017

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