| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
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. cc @AA-Turner |
Sorry, something went wrong.
Maybe I should just commit the generated .svg (and the input dot file so it can be revised easily). Would that be acceptable? |
Sorry, something went wrong.
* 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 .svg so that the substance of this PR can be reviewed while we figure out if it is acceptable to add graphviz as a dependency. (Note that sphinx.ext.graphviz is a built-in extension, so enabling it doesn't add any new sphinx dependencies.) |
Sorry, something went wrong.
|
I think that requiring graphviz should be fine -- Debian, Fedora, Gentoo, and OpenSUSE all package it. As Richard notes, it's a built-in extension, so should be fine from the "Vanilla" perspective. I would want to include a NEWS entry to say that graphviz is now required to build the docs, though. A |
Sorry, something went wrong.
|
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 the InternalDocs folder instead. It's also worth noting here that tp_finalize isn't 100% related to garbage collection, it's supposed to be used over tp_dealloc if complicated things are being done upon finalization, even for non-GC types. And while we're here, I think it would be a good idea to document the cases that tp_clear should exist for a tracked type. |
Sorry, something went wrong.
This reverts commit 361eaca.
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, tp_finalize is never called for non-GC types unless the class designer calls it from tp_dealloc. In that case tp_finalize is just like any other helper function that might be called from tp_dealloc. (Maybe this is only true for static types and not heap types? I don't fully understand the difference.)
I thought that was already sufficiently explained, even before this PR. Can you explain what you think is lacking? |
Sorry, something went wrong.
|
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 that all types, even GC, require PyObject_CallFinalizerFromDealloc in the destructor if they want tp_finalize to get eventually called--we can note that it could happen automatically, though.
Basically, it's not documented which types need to have a tp_clear, because not all GC types have it. I'm not even sure which cases require it. I think it's only needed if the type can have a direct reference cycle to itself? (As in, running its finalizer will try to Py_DECREF itself.) Also, I don't think it should be documented that tp_clear is related to tp_dealloc by making an "optional call", that's sort of incidental. They tend to do the same thing, and the destructor can utilize the clear function for convenience, but they're for different purposes. A few other notes:
|
Sorry, something went wrong.
|
I did a major rewrite to hopefully address all of the feedback (thanks for reviewing!). Please take another look. |
Sorry, something went wrong.
|
I'm going to be taking this over. I've addressed my own comments and most of Petr's. Other reviews would be appreciated! |
Sorry, something went wrong.
|
Thank you! I definitely want to take another look -- after Beta 1, next week or so. |
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sounds good to me. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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 the attached 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.) |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot iOS ARM64 Simulator 3.x (tier-3) has failed when building commit 3246ea5. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1380/builds/3754 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/thread.py", line 99, in _worker
ctx.initialize()
~~~~~~~~~~~~~~^^
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/interpreter.py", line 132, in initialize
self.interpid = _interpreters.create(reqrefs=True)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
interpreters.InterpreterError: interpreter creation failed
ERROR
Traceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/test/test_concurrent_futures/executor.py", line 127, in test_map_buffersize_on_infinite_iterable
self.assertEqual(next(res, None), "0")
~~~~^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/_base.py", line 666, in result_iterator
yield _result_or_cancel(fs.pop())
~~~~~~~~~~~~~~~~~^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/_base.py", line 311, in _result_or_cancel
return fut.result(timeout)
~~~~~~~~~~^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/_base.py", line 450, in result
return self.__get_result()
~~~~~~~~~~~~~~~~~^^
File "/Users/buildbot/Library/Developer/XCTestDevices/4832ED11-0C79-4767-B1A6-77FDBE8E03AE/data/Containers/Bundle/Application/95646D17-A682-4984-91B0-88B196595790/iOSTestbed.app/python/lib/python3.15/concurrent/futures/_base.py", line 395, in __get_result
raise self._exception
concurrent.futures.interpreter.BrokenInterpreterPool: A thread initializer failed, the thread pool is not usable anymore
|
Sorry, something went wrong.
|
I'd go for 3.14; that'll become /3/ in this October. |
Sorry, something went wrong.
…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 commit 3246ea5) 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 the 3.14 branch. |
Sorry, something went wrong.
…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 commit 3246ea5) Co-authored-by: Richard Hansen <rhansen@rhansen.org> Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
…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 Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
…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 Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
| Back | FazBrowse Home | New Git URL |
📚 Documentation preview 📚: https://cpython-previews--125962.org.readthedocs.build/