◐ Shell
reader mode source ↗
Skip to content

bpo-33330: PyImport_Cleanup check for exc leaks#7068

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

bpo-33330: PyImport_Cleanup check for exc leaks#7068
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:pyimport_cleanup_assert

Conversation

@vstinner

@vstinner vstinner commented May 23, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  • Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup()
    and _PyGC_CollectNoFail() to make sure that these functions don't
    leak exceptions.
  • Replace also PyImport_GetModuleDict() call with interp->modules to
    prevent a fatal error if modules is already NULL.

https://bugs.python.org/issue33330

@vstinner
bpo-33330: PyImport_Cleanup check for exc leaks …
52b8028 
* Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup()
  and _PyGC_CollectNoFail() to make sure that these functions don't
  leak exceptions.
* Replace also PyImport_GetModuleDict() call with interp->modules to
  prevent a fatal error if modules is already NULL.
@vstinner

vstinner commented May 23, 2018

Copy link
Copy Markdown
Member Author

The commit d393c1b replaced interp->modules with PyImport_GetModuleDict(), but this function fails with a fatal error if modules is NULL. I'm not sure that it was a deliberate change. Moreover, this commit has been partially reverted (interp->modules is back!).

@vstinner

vstinner commented May 23, 2018

Copy link
Copy Markdown
Member Author

PyImport_Cleanup() is part of the public API, but it's documented as "Empty the module table. For internal use only." :-( Why do we have public API for internal use only? Likely a mistake from the past.

@serhiy-storchaka

serhiy-storchaka commented May 23, 2018

Copy link
Copy Markdown
Member

I would left only few asserts after calls that can raise exceptions if the third-party code has bugs. And one at the end of PyImport_Cleanup() just for sure. For other asserts we can guarantee the absence of exceptions.

@vstinner

vstinner commented May 23, 2018

Copy link
Copy Markdown
Member Author

The last 2 years, I added many "assert(!PyErr_Occurred());" in ceval.c and object.c, and it helped me a lot to detect some very old bugs. The rationale here is the same.

@serhiy-storchaka

serhiy-storchaka commented May 26, 2018

Copy link
Copy Markdown
Member

See also #7126.

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

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

Hide comment

LGTM, except that GC is covered by a separate PR.

@vstinner

vstinner commented May 28, 2018

Copy link
Copy Markdown
Member Author

LGTM, except that GC is covered by a separate PR.

I will wait until #7126 is merged, and then rebase this PR on top of it.

@vstinner vstinner closed this Jul 16, 2018
@vstinner vstinner deleted the pyimport_cleanup_assert branch July 16, 2018 14:51
@serhiy-storchaka

serhiy-storchaka commented Jul 16, 2018

Copy link
Copy Markdown
Member

Why not rebase and merge this PR?

@vstinner

vstinner commented Jul 16, 2018

Copy link
Copy Markdown
Member Author

It's just that I wrote it 2 months ago, and I lost interest in this minor enhancement in the meanwhile. I'm trying to reduce my number of open PRs. Feel free to reuse my patch to create a new PR if you want.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Jun 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

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