fix: clear local internals after finalizing interpreter #2101#3744
fix: clear local internals after finalizing interpreter #2101#3744rwgk merged 3 commits intopybind:masterfrom
Conversation
henryiii
left a comment
henryiii
left a comment
There was a problem hiding this comment.
Seems fine to me, though I don't use this functionality much. @Skylion007 or @rwgk?
+1 I could run this through Google's global testing, then merge if there are no issues. There is a chance that even the global testing doesn't hit this change, embedding is on our "don't use" list. But this PR looks good to me, nice test. |
Skylion007
left a comment
Skylion007
left a comment
There was a problem hiding this comment.
This looks good to me. Although I would appreciate in a comment in the test which links back to the issue, and maybe another comment in embed.h about why clearing the interpreters is necessary.
for more information, see https://pre-commit.ci
It's done. I'm sorry for the extra commit. |
|
The more the merrier. They get squashed anyway when we merge. I'll try to
get this globally tested asap.
…On Sat, Feb 19, 2022 at 15:18 StarQTius ***@***.***> wrote:
This looks good to me. Although I would appreciate in a comment in the
test which links back to the issue, and maybe another comment in embed.h
about why clearing the interpreters is necessary.
It's done. I'm sorry for the extra commit.
—
Reply to this email directly, view it on GitHub
<#3744 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFUZAC3P5AKL5NH2F24VX3U4AQNRANCNFSM5OTG26AA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Global testing went fine.
I actually ran it twice:
- Exactly this PR: everything worked (10s of millions of tests).
- With an added
pybind11_failin pybind11/embed.h: exactly 1 failure (this one fwiw).
Learned:
- This PR is good.
- I'm beginning to think adding test_embed to the build config for internal testing isn't difficult. (But later, maybe.)
| // Local internals contains data managed by the current interpreter, so we must clear them to | ||
| // avoid undefined behaviors when initializing another interpreter | ||
| detail::get_local_internals().registered_types_cpp.clear(); | ||
| detail::get_local_internals().registered_exception_translators.clear(); |
There was a problem hiding this comment.
Just to note: I verified that new TEST_CASE covers .registered_types_cpp.clear(), but it does not cover .registered_exception_translators.clear().
I think that's fine though, although if someone could add that test coverage in a follow-on PR that would be ideal.
** "covers" in the sense that the test breaks if the call is removed.
…ybind#3744) * Clear local internals after finalizing interpreter * Add descriptive comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Clear local internals after finalizing interpreter * Add descriptive comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
Description
The author of #2101 proposed to clear the local internals after finalizing an interpreter. It seems that when registering a type as local, some of the data is managed by the current interpreter through pointers, which are then dangling when the interpreter is finalized. I implemented the solution suggested by the author, as it does not seem that keeping the local internals between two consecutive interpreters is the expected behavior.
Suggested changelog entry: