Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-126907: Use a list foratexit 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
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
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
3b76682 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Congrats@ZeroIntensity for this nice fix! |
…127935)Co-authored-by: Victor Stinner <vstinner@python.org>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
…127935)Co-authored-by: Victor Stinner <vstinner@python.org>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
…127935)Co-authored-by: Victor Stinner <vstinner@python.org>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
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 PRdescription below. --><!-- ^ For example, instead of "Fixessagemath#12345" use "Introduce new methodto 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. Forexample, "Fixessagemath#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 Carray- The array can be directly accessed from Cython code using pointerarithmetic### Python 3.14 Implementation:- The ``atexit`` module was refactored to use a Python ``PyList`` objectinstead 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 documentationpreview.### ⌛ 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@3b7668284d4461afpython/cpython#127935 URL:sagemath#41021Reported by: Chenxin ZhongReviewer(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 PRdescription below. --><!-- ^ For example, instead of "Fixessagemath#12345" use "Introduce new methodto 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. Forexample, "Fixessagemath#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 Carray- The array can be directly accessed from Cython code using pointerarithmetic### Python 3.14 Implementation:- The ``atexit`` module was refactored to use a Python ``PyList`` objectinstead 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 documentationpreview.### ⌛ 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@3b7668284d4461afpython/cpython#127935 URL:sagemath#41021Reported by: Chenxin ZhongReviewer(s): Copilot, da-woods
Uh oh!
There was an error while loading.Please reload this page.
cc@colesbury,@vstinner,@kumaraditya303
This is an alternative togh-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