-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
[3.12] gh-116510: Fix a crash due to shared immortal interned strings. #124541
Conversation
Fix a crash caused by immortal interned strings being shared between sub-interpreters that use basic single-phase init. In that case, the string can be used by an interpreter that outlives the interpeter that created and interned it. For interpreters that share obmalloc state, also share the interned dict with the main interpreter.
This is a small change code-wise but I'm not sure about the full implications of it. It does fix a bug but might create other bugs or problems, I'm not confident. One downside to this change is that if many sub-interpreters are created and they create a lot of immortal interned strings, those strings will remain alive in the main interpreter state. So, it could cause a lot more memory to be used. My gut feeling is that's okay since we shouldn't be immortalizing strings unless we are fairly sure they won't cause memory bloat, even in a single interpreter case. E.g. it happens to variable names appearing in Python source code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM
I think there are just a couple spots where we need to clear the PyInterpreterState
field for the shared interned dict case.
When you're done making the requested changes, leave the comment: |
I have made the requested changes; please review again |
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
…terned strings. (pythongh-124541)" This reverts commit 5dd07eb.
GH-124814 is a backport of this pull request to the 3.12 branch. |
Fix a crash caused by immortal interned strings being shared between sub-interpreters that use basic single-phase init. In that case, the string can be used by an interpreter that outlives the interpeter that created and
interned it. For interpreters that share obmalloc state, also share the interned dict with the main interpreter.