-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Cursor-generated: Add leak_detach path for internals after shutdown #5966
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
+72
−57
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Py_IsInitialized/Py_IsFinalizing are runtime-wide, not per-interpreter, and Py_IsFinalizing stays set after Py_Finalize until the next init. Using those checks to decide whether to DECREF internals can skip cleanup or touch the C-API after a specific interpreter is gone. Add internals::leak_detach() and local_internals::leak_detach() to clear the owned PyType/PyObject pointers without calling into Python, and invoke that from internals_pp_manager::destroy() before deleting the pp in the post- Py_Finalize/Py_EndInterpreter paths. The destructors now always Py_CLEAR when invoked during state-dict teardown, preserving cleanup of the pybind11 heap types while avoiding UB in late-destroy scenarios. Cursor GPT-5.2 Codex Extra High ┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ → Could you please read this file for context: /wrk/logs/gh_pr_info_pybind_pybind11_5958_2026-01-19+100747.md │ │ │ │ In my last comment there I wrote: I'll point Cursor to this, the pybind11 sources, and the CPython 3.14 sources, and ask it to look very thoroughly to figure out what is the │ │ best achievable solution in terms of avoiding UB but leaking as little as possible. │ │ │ │ The up-to-date pybind11 sources are right here (master branch). │ │ │ │ The CPython sources are here: │ │ │ │ smc120-0009.ipp2a2.colossus.nvidia.com:/wrk/clone/cpython $ git branch │ │ * 3.14 │ │ main │ │ v3.14.2_release │ │ │ │ JIC we need this for your analysis: │ │ │ │ I last git pulled the CPython 3.14 branch yesterday and built from sources using this script: │ │ │ │ /home/rgrossekunst/rwgk_config/bin/build_cpython_from_git_twice.sh │ │ │ │ The installations are here: │ │ │ │ /wrk/cpython_installs/3.14_branch_23e3c0ae867_default │ │ /wrk/cpython_installs/3.14_branch_23e3c0ae867_freethreaded │ │ │ │ Could you please "look very thoroughly to figure out what is the best achievable solution in terms of avoiding UB but leaking as little as possible.", also taking Joshua's "I │ │ think the correct solution is a internals::leak_detach() method" comment into account? │ │ │ │ Please use the cursor_workspace/ subdirectly here for all intermediate/temporary files. Do NOT use /tmp. │ │ │ │ If you want to rebuild the pybind11 unit tests for your analysis, please do NOT use cmake. Use this command instead: │ │ │ │ cd /wrk/bld/pybind11_gcc_v3.14.2_df793163d58_default/ && scons -j $(nproc) && TestVenv/bin/python3 ../../clone/pybind11_scons/run_tests.py ../../forked/pybind11 24 │ │ │ │ Currently I want to focus on the "default" cpython build. We can leave any "freethreaded" considerations for later. │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Collaborator
Author
|
I'm dumping here what Cursor showed on the screen: |
Fold b-pass PR pybind#5965's per-interpreter ownership checks into internals and local_internals while keeping leak_detach for post-teardown destroy() paths. Add a non-null guard so a nullptr return from get_interpreter_state_unchecked() late in shutdown can't match nullptr istate and trigger Py_CLEAR without an active interpreter (avoid UB). Keep helper placement aligned with PR pybind#5965.
Collaborator
Author
|
Closing in favor of #5965 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Py_IsInitialized/Py_IsFinalizing are runtime-wide, not per-interpreter, and Py_IsFinalizing stays set after Py_Finalize until the next init. Using those checks to decide whether to DECREF internals can skip cleanup or touch the C-API after a specific interpreter is gone.
Add internals::leak_detach() and local_internals::leak_detach() to clear the owned PyType/PyObject pointers without calling into Python, and invoke that from internals_pp_manager::destroy() before deleting the pp in the post- Py_Finalize/Py_EndInterpreter paths. The destructors now always Py_CLEAR when invoked during state-dict teardown, preserving cleanup of the pybind11 heap types while avoiding UB in late-destroy scenarios.
Cursor GPT-5.2 Codex Extra High
Suggested changelog entry: