-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-126907: Use a list for atexit callbacks
#127935
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
Conversation
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.
I like the overall change. Here is a first review.
Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
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
Co-authored-by: Victor Stinner <[email protected]>
|
Congrats @ZeroIntensity for this nice fix! |
…127935) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…127935) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…127935) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
|
Should this be backported or is it not worth it? |
|
To 3.13? I don't think it's worth risking breakage. |
sagemathgh-41021: Refactor ``atexit.pyx`` <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> I have refactor the ``atexit.pyx`` since for python 3.14, the changes of ``atexit`` is so much for supporting the nogil version. ### Python<=3.13 Implementation: - The ``atexit`` module stores callbacks in a C array of structs (``atexit_callback``). - The structure is defined in C as: ``` typedef struct { PyObject *func; PyObject *args; PyObject *kwargs ;} atexit_callback; ``` - Callbacks are stored in the interp->atexit.callbacks field as a C array - The array can be directly accessed from Cython code using pointer arithmetic ### Python 3.14 Implementation: - The ``atexit`` module was refactored to use a Python ``PyList`` object instead of a C array. - The structure is now: ``` state.callbacks = [(func, args, kwargs), ...] # A Python list of tuples ``` - In the C implementation, callbacks are managed with: ``` PyObject *callbacks; // This is now a PyList ``` - Callbacks are inserted at the beginning (LIFO order) using ``PyList_Insert(state->callbacks, 0, callback)`` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> python/cpython@3b76682 84d4461af python/cpython#127935 URL: sagemath#41021 Reported by: Chenxin Zhong Reviewer(s): Copilot, da-woods
sagemathgh-41021: Refactor ``atexit.pyx`` <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> I have refactor the ``atexit.pyx`` since for python 3.14, the changes of ``atexit`` is so much for supporting the nogil version. ### Python<=3.13 Implementation: - The ``atexit`` module stores callbacks in a C array of structs (``atexit_callback``). - The structure is defined in C as: ``` typedef struct { PyObject *func; PyObject *args; PyObject *kwargs ;} atexit_callback; ``` - Callbacks are stored in the interp->atexit.callbacks field as a C array - The array can be directly accessed from Cython code using pointer arithmetic ### Python 3.14 Implementation: - The ``atexit`` module was refactored to use a Python ``PyList`` object instead of a C array. - The structure is now: ``` state.callbacks = [(func, args, kwargs), ...] # A Python list of tuples ``` - In the C implementation, callbacks are managed with: ``` PyObject *callbacks; // This is now a PyList ``` - Callbacks are inserted at the beginning (LIFO order) using ``PyList_Insert(state->callbacks, 0, callback)`` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> python/cpython@3b76682 84d4461af python/cpython#127935 URL: sagemath#41021 Reported by: Chenxin Zhong Reviewer(s): Copilot, da-woods
cc @colesbury, @vstinner, @kumaraditya303
This is an alternative to gh-126908, and I'm a lot happier with this. Sam's suggestion of using a list turned out to be pretty nice, with the exception of
unregisterbeing a little wonky. I suspect we could improve that a little by adding a private API for removing from a list more cleanly, but that's work for later. FWIW, both this PR and the other one will have trouble backporting due to the runtime structure changing size.atexitfrom threads in free-threading build segfaults #126907