Uh oh!
There was an error while loading. Please reload this page.
gh-140550: Update xxlimited with 3.15 limited API & PEP 697 - #142827
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.
Uh oh!
There was an error while loading. Please reload this page.
| /* TODO: This is not quite true yet: there is a race in Xxo_setattro | ||
| * for example. | ||
| */ |
There was a problem hiding this comment.
Wouldn't it be better to mark this as Py_MOD_GIL_USED in the meantime, or are you trying to avoid the warning?
There was a problem hiding this comment.
AFAIK, the situation is unchanged since Py_MOD_GIL_NOT_USED was added everywhere in #116882.
I think the intention is/was to make everything thread-safe before some stage of free-threading implementation?
There was a problem hiding this comment.
I think we added that everywhere under the assumption that everything was already thread-safe, and anything that should be reported to us as a bug. I'm not sure that applies with our own test suite.
There was a problem hiding this comment.
This is not really a test suite, it supposed to be an example/template showing how to use the Limited API.
The limited API was never compatible with free-threading (yet), so I don't think there's a bug?
There's an earlier PR, #110764, which did this (Py_NOGIL was later renamed to Py_GIL_DISABLED):
+#ifndef Py_NOGIL
#define Py_LIMITED_API 0x030d0000
+#endif... but that seems to defeat the point of this module :/
Maybe we should have not built these modules at all in free-threading builds, and skip the relevant tests?
Doing that now seems like wasted effort though, when mutexes/critical sections are a genuine TODO item for the limited API.
There was a problem hiding this comment.
If it's a template, I'm a little concerned that people will see this and think "oh, this module is thread-safe", when it's really not. The comment helps, but I don't know what percentage of people will ignore it.
There was a problem hiding this comment.
Before PyCriticalSection is added to the Stable ABI, we can use PyThread_acquire_lock for per-instance locking. (It's also a nice example of a non-PyObject resource that needs freeing.)
encukou
commented
Dec 16, 2025
I'll mark this draft -- this PR makes some shortcomings quite visible, and I guess it's better to address them before suggesting the new API :) |
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.
| // traverse: Visit all references from an object, including its type | ||
| static int | ||
| Xxo_traverse(PyObject *op, visitproc visit, void *arg) | ||
| Xxo_traverse(PyObject *self, visitproc visit, void *arg) |
There was a problem hiding this comment.
@colesbury, regarding your post on Discourse:
I think that’s just
PyType_GetBaseByTokenandPyType_GetModuleByToken. It seems pretty rare to need to call them from a tp_traverse handler. If really necessary, I think you can work around that in extensions by calling other APIs.
How would you implement the traverse function here?
Xxo is a subclassable type, and it should work on ABIs with different sizeof(PyObject) (i.e. free-threaded & GIL-ful).
There was a problem hiding this comment.
By workaround I mean something like:
static Xxo_Type_spec = {
...
.flags = Py_TPFLAGS_IMMUTABLETYPE | ...
};
...
static PyTypeObject *Xxo_Type;
...
Xxo_Type = state->Xxo_Type = (PyTypeObject*)PyType_FromModuleAndSpec(...);
static XxoObject_Data *
Xxo_get_data(PyObject *self)
{
return PyObject_GetTypeData(self, Xxo_Type);
}
We could also avoid all this if we had a PyObject_GetTypeDataByToken().
There was a problem hiding this comment.
static PyTypeObject *Xxo_Type;
...
Xxo_Type = state->Xxo_Type = (PyTypeObject*)PyType_FromModuleAndSpec(...);
That'll overwrite the pointer in a subinterpreter, or if you otherwise load several copies of the module. It's not something we can recommend.
| } | ||
| if (PyModule_AddType(m, (PyTypeObject*)Str_Type) < 0) { | ||
| if (PyModule_AddType(m, Str_Type) < 0) { | ||
| return -1; |
There was a problem hiding this comment.
Don't we have to call Py_DECREF(Str_Type) here (on error)?
| PySlot_FUNC(Py_mod_exec, xx_modexec), | ||
| /* Module state and associated functions */ | ||
| PySlot_SIZE(Py_mod_state_size, (void*)sizeof(xx_state)), |
There was a problem hiding this comment.
I don't think that the (void*) cast is needed here:
| PySlot_SIZE(Py_mod_state_size, (void*)sizeof(xx_state)), | |
| PySlot_SIZE(Py_mod_state_size, sizeof(xx_state)), |
| static PySlot xx_slots[] = { | ||
| /* Basic metadata */ | ||
| PySlot_STATIC_DATA(Py_mod_name, "xxlimited"), | ||
| PySlot_STATIC_DATA(Py_mod_doc, (void*)module_doc), |
There was a problem hiding this comment.
Is the (void*) cast useful here?
| PySlot_STATIC_DATA(Py_mod_doc, (void*)module_doc), | |
| PySlot_STATIC_DATA(Py_mod_doc, module_doc), |
Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.15. |
GH-149785 is a backport of this pull request to the 3.15 branch. |
xxlimitedtoxxlimited_3_13: this module serves as a rudimentary check that we don't break older Limited API. This is similar toxxlimited_35. (Nowadays we know that it's good to have a separator between the major/minor versions, but renaming toxxlimited_3_5isn't worth the churn.)test_xxlimitedto make similar copies easier in the futurexxlimitedto Limited API 3.15,PyModExport(PEP-793), standalone instance struct with extended basicsize (PEP-697)_Py_OPAQUE_PYOBJECT) for now to ensure we don't rely onPyObjectlayout details accidentally. (I'll replace/remove the private API in 3.15 beta at latest.)