◐ Shell
reader mode source ↗
Skip to content

bpo-33622: Add checks for exceptions leaks in the garbage collector.#7126

Merged
serhiy-storchaka merged 3 commits into
python:masterfrom
serhiy-storchaka:gc
May 29, 2018
Merged

bpo-33622: Add checks for exceptions leaks in the garbage collector.#7126
serhiy-storchaka merged 3 commits into
python:masterfrom
serhiy-storchaka:gc

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented May 26, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Failure in adding to gc.garbage is no longer fatal.

https://bugs.python.org/issue33622

@serhiy-storchaka
bpo-33622: Add checks for exceptions leaks in the garbage collector. …
27f2b83 
Failure in adding to gc.garbage is no longer fatal.
@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement skip news labels May 26, 2018
@serhiy-storchaka serhiy-storchaka mentioned this pull request May 26, 2018
Closed
vstinner
vstinner approved these changes May 28, 2018
View reviewed changes

@vstinner vstinner 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.

Hide comment

LGTM.

My comments are just minor suggestions. The current change is good, if you want to apply it as it is.

serhiy-storchaka
serhiy-storchaka commented May 28, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hide comment

Thank you Victor! I hoped on your review.

vstinner
vstinner approved these changes May 28, 2018
View reviewed changes

@vstinner vstinner 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.

Hide comment

The PR is now perfect :-D

Exception handling at the C level is hard :-( It's so easy to get it wrong (clear or replace the current exception by mistake). These assertions should help to detect such bugs earlier.

vstinner
vstinner approved these changes May 29, 2018
View reviewed changes

@vstinner vstinner 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.

Hide comment

Still LGTM even with the latest change ;-)

@serhiy-storchaka

serhiy-storchaka commented May 29, 2018

Copy link
Copy Markdown
Member Author

I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which tp_clear() sets an exception. Currently this can lead to a crash. Cases in the stdlib should be fixed in separate issues, but there may be third-party code.

@serhiy-storchaka serhiy-storchaka merged commit c4653c9 into python:master May 29, 2018
@serhiy-storchaka serhiy-storchaka deleted the gc branch May 29, 2018 15:50
@vstinner

vstinner commented May 29, 2018

Copy link
Copy Markdown
Member

I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which tp_clear() sets an exception. Currently this can lead to a crash.

Oh. It's a bug, right? Do you plan to open a new issue or write a fix for these bugs?

If the bug is "very unlikely" and the case is now handled properly (error logged into stderr), maybe it's fine.

@serhiy-storchaka

serhiy-storchaka commented May 31, 2018

Copy link
Copy Markdown
Member Author

I have opened three new issues for three possible cases of exceptions in tp_clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Assignees

No one assigned

Labels

skip news type-feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@serhiy-storchaka @vstinner @the-knights-who-say-ni @bedevere-bot