Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-140550: Update xxlimited with 3.15 limited API & PEP 697#142827
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
base:main
Are you sure you want to change the base?
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.
| XxoObject*self=XxoObject_CAST(op); | ||
| Py_VISIT(self->x_attr); | ||
| XxoObject_Data*data=Xxo_get_data(self); | ||
| if (data==NULL) { |
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.
Xxo_get_data will set an exception on failure, which will probably break the GC. Same forXxo_clear below.
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.
Yeah; thenewly documented requirements fortp_traverse are breaking the design; I'll need to add some new borrowing+infallible APIs :/
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.
Maybe call PyErr_WriteUnraisable()?
Same for Xxo_finalize(), no? Py_tp_finalize is not supposed to exit with an exception set._Py_Dealloc() (tp_del) checks if an exception is set if Python is built in debug mode, butPyObject_CallFinalizer() (tp_finalize) doesn't.
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.
PyErr_WriteUnraisable won't work, becausetp_finalize can't create an exception object at all. It mustnot touch any refcounts; it's not enough to restore them to the previous state.
This is a problem. See:https://discuss.python.org/t/adding-c-api-for-use-in-tp-traverse/105331
Modules/xxlimited.c Outdated
| /* TODO: This is not quite true yet: there is a race in Xxo_setattro | ||
| * for example. | ||
| */ |
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.
Wouldn't it be better to mark this asPy_MOD_GIL_USED in the meantime, or are you trying to avoid the warning?
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.
AFAIK, the situation is unchanged sincePy_MOD_GIL_NOT_USED was added everywhere in#116882.
I think the intention is/was to make everything thread-safe beforesome stage of free-threading implementation?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 toPy_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 thatnow 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
encukou commentedDec 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 :) |
Modules/xxlimited.c Outdated
| newXxoObject(PyObject*module) | ||
| // Xxo initialization | ||
| // This is the implementation of Xxo.__new__; it takes arbitrary positional | ||
| // and keyword arguments. |
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 don't understand "it takes arbitrary positional and keyword arguments" since the error message below is "Xxo.__new__() takes no arguments".
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.
Thetp_new calling convention takes args&kwargs, so we need to check.
I agree the comment was misleading. Also the situation should be clear without the comment, so I removed it.
| XxoObject*self=XxoObject_CAST(op); | ||
| Py_VISIT(self->x_attr); | ||
| XxoObject_Data*data=Xxo_get_data(self); | ||
| if (data==NULL) { |
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.
Maybe call PyErr_WriteUnraisable()?
Same for Xxo_finalize(), no? Py_tp_finalize is not supposed to exit with an exception set._Py_Dealloc() (tp_del) checks if an exception is set if Python is built in debug mode, butPyObject_CallFinalizer() (tp_finalize) doesn't.
Modules/xxlimited.c Outdated
| if (self->x_attr==NULL) { | ||
| // filter a specific attribute name | ||
| intis_reserved=PyUnicode_EqualToUTF8(name,"reserved"); | ||
| if (is_reserved<0) { |
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.
PyUnicode_EqualToUTF8() cannot return-1:https://docs.python.org/dev/c-api/unicode.html#c.PyUnicode_EqualToUTF8AndSize
Uh oh!
There was an error while loading.Please reload this page.
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.)