Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-75459: Doc: C API: Improve object life cycle documentation#125962
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
I'm not familiar enough with CPython's internals to be super confident about these changes. I would appreciate it if a GC expert would carefully review this. Thanks! |
Hmm, I'm not sure if we can require graphviz for the docs. We'd have to consider installing it on the main docs server in addition to Read the Docs, and also make sure the docs can still build without it, for downstream redistributors who might only want to build with "vanilla" Sphinx and no extra extensions. Plus other developers would need an easy way to build the docs on their machines. |
be22691
tod804c6c
CompareUh oh!
There was an error while loading.Please reload this page.
Maybe I should just commit the generated |
* Add "cyclic isolate" to the glossary. * Add a new "Object Life Cycle" page. * Illustrate the order of life cycle functions. * Document `PyObject_CallFinalizer` and `PyObject_CallFinalizerFromDealloc`. * `PyObject_Init` does not call `tp_init`. * `PyObject_New`: * also initializes the memory * does not call `tp_alloc`, `tp_new`, or `tp_init` * should not be used for GC-enabled objects * memory must be freed by `PyObject_Free` * `PyObject_GC_New` memory must be freed by `PyObject_GC_Del`. * Warn that garbage collector functions can be called from any thread. * `tp_finalize` and `tp_clear`: * Only called when there's a cyclic isolate. * Only one object in the cyclic isolate is finalized/cleared at a time. * Clearly warn that they might not be called. * They can optionally be manually called from `tp_dealloc` (via `PyObject_CallFinalizerFromDealloc` in the case of `tp_finalize`). * `tp_finalize`: * Reference `object.__del__`. * The finalizer can resurrect the object. * Suggest `PyErr_GetRaisedException` and `PyErr_SetRaisedException` instead of the deprecated `PyErr_Fetch` and `PyErr_Restore` functions. * Add links to `PyErr_GetRaisedException` and `PyErr_SetRaisedException`. * Suggest using `PyErr_WriteUnraisable` if an exception is raised during finalization. * Rename the example function from `local_finalize` to `foo_finalize` for consistency with the `tp_dealloc` documentation and as a hint that the name isn't special. * Minor wording and sylistic tweaks. * Warn that `tp_finalize` can be called during shutdown.
I committed the generated |
I think that requiring I would want to include a NEWS entry to say that A |
Uh oh!
There was an error while loading.Please reload this page.
My main concern with documenting nitty-gritty details of the lifecycle is that we're technically documenting implementation details, which are subject to change (and we've been bad at updating these kind of things from version-to-version in past). I suggest the SVG go into theInternalDocs folder instead. It's also worth noting here that |
This reverts commit361eaca.
I don't want to document any implementation details here, so I'm happy to remove what isn't necessary. It's hard to tell what is and isn't necessary because the end of an object's life is especially fraught with peril. I think that it is better to err on the side of over-documenting this topic than under-documenting. I wrote this PR because there were several things that I needed to know that the existing documentation didn't make clear:
If I understand correctly,
I thought that was already sufficiently explained, even before this PR. Can you explain what you think is lacking? |
First, thanks for doing this!
I don't, that limits our ability to modify the lifecycle in the future (especially because there's no good way to deprecate things here). I'll point this out when doing a more in-depth review though, I don't see anything particularly bad right now.
You're right, it's not, but I don't think we should limit ourselves to that in the future. It might be possible someday to automatically do this for untracked types as well. We should just document thatall types, even GC, require
Basically, it's not documented which types need to have a Also, I don't think it should be documented that A few other notes:
|
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.
I did a major rewrite to hopefully address all of the feedback (thanks for reviewing!). Please take another look. |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
I'm going to be taking this over. I've addressed my own comments and most of Petr's. Other reviews would be appreciated! |
Uh oh!
There was an error while loading.Please reload this page.
Thank you! I definitely want to take another look -- after Beta 1, next week or so. |
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.
Overall, this is an improvement. Thank you for the work!
Let's do one more pass, merge, and leave the rest for a future PR?
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: Petr Viktorin <encukou@gmail.com>
Sounds good to me. |
3246ea5
intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks, Petr! Do we want to backport this? On the one hand, this is a great improvement to the C API documentation that can definitely help users in 3.13. But on the other day, this is a huge change to backport. Maybe that doesn't matter? I'm not sure we should treat documentation changes the same way we treat code changes. |
Very few people read the pre-release docs. Over the past week, we had these many visitors for these homepages:
If we want people to benefit from this work before October 2026(!), we should backport. |
To 3.14, or all the way to 3.13? I can see arguments for both. Note that if we backport to 3.13, we also need to backport theattached thread state change that this PR uses. (That might be a good thing, I've noticed myself constantly pointing free-threading users to the 3.14 docs for the better documentation.) |
bedevere-bot commentedMay 20, 2025
|
I'd go for 3.14; that'll become |
…ythonGH-125962) * Add "cyclic isolate" to the glossary. * Add a new "Object Life Cycle" page. * Improve docs for related API, with special focus on cross-references and warnings(cherry picked from commit3246ea5)Co-authored-by: Richard Hansen <rhansen@rhansen.org>Co-authored-by: Petr Viktorin <encukou@gmail.com>Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
GH-134344 is a backport of this pull request to the3.14 branch. |
…H-125962) (GH-134344)gh-75459: Doc: C API: Improve object life cycle documentation (GH-125962) * Add "cyclic isolate" to the glossary. * Add a new "Object Life Cycle" page. * Improve docs for related API, with special focus on cross-references and warnings(cherry picked from commit3246ea5)Co-authored-by: Richard Hansen <rhansen@rhansen.org>Co-authored-by: Petr Viktorin <encukou@gmail.com>Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
PyObject_CallFinalizer
andPyObject_CallFinalizerFromDealloc
.PyObject_Init
does not calltp_init
.PyObject_New
:tp_alloc
,tp_new
, ortp_init
PyObject_Free
PyObject_GC_New
memory must be freed byPyObject_GC_Del
.tp_finalize
andtp_clear
:tp_dealloc
(viaPyObject_CallFinalizerFromDealloc
in the case oftp_finalize
).tp_finalize
:object.__del__
.PyErr_GetRaisedException
andPyErr_SetRaisedException
instead of the deprecatedPyErr_Fetch
andPyErr_Restore
functions.PyErr_GetRaisedException
andPyErr_SetRaisedException
.PyErr_WriteUnraisable
if an exception is raised during finalization.local_finalize
tofoo_finalize
for consistency with thetp_dealloc
documentation and as a hint that the name isn't special.tp_finalize
can be called during shutdown.📚 Documentation preview 📚:https://cpython-previews--125962.org.readthedocs.build/