| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks @ericsnowcurrently! I can verify that this solves numpy/numpy#23766.
Sorry, something went wrong.
|
Is it possible to use a reduced variant of the numpy case as a regression test? |
Sorry, something went wrong.
|
Without knowing too much around the issue, I was able to come up with this, which is really close to what numpy does (not sure how we would go about adding a test for it). #define PY_SSIZE_T_CLEAN
#include "Python.h"
static PyTypeObject CustomBaseType1 = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Base1",
.tp_basicsize = sizeof(PyObject),
};
static PyTypeObject CustomBaseType2 = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Base2",
.tp_basicsize = sizeof(PyObject),
};
static PyTypeObject CustomDerivedType = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "custom.Derived",
.tp_basicsize = sizeof(PyObject),
};
static PyModuleDef custommodule = {
PyModuleDef_HEAD_INIT,
.m_name = "custom",
.m_doc = "Example module that creates an extension type.",
.m_size = -1,
};
PyMODINIT_FUNC
PyInit_custom(void)
{
PyObject *m;
if (PyType_Ready(&CustomBaseType1) < 0)
return NULL;
if (PyType_Ready(&CustomBaseType2) < 0)
return NULL;
CustomDerivedType.tp_base = &CustomBaseType2;
CustomDerivedType.tp_bases =
Py_BuildValue("(OO)", &CustomBaseType2, &CustomBaseType1);
CustomDerivedType.tp_hash = CustomBaseType1.tp_hash;
if (PyType_Ready(&CustomDerivedType) < 0)
return NULL;
m = PyModule_Create(&custommodule);
if (m == NULL)
return NULL;
Py_INCREF(&CustomDerivedType);
if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomDerivedType) < 0) {
Py_DECREF(&CustomDerivedType);
Py_DECREF(m);
return NULL;
}
return m;
}It segfaults on import on main, and is okay with this PR. |
Sorry, something went wrong.
|
Would it make sense to allocate the tuples with malloc at start up, and never free them? There would be no need to free them at the end of the interpreters lifetime, and there would be no issue if an interpreter other than the main interpreter outlives the main interpreter. |
Sorry, something went wrong.
Yeah. However, that's orthogonal to this PR. |
Sorry, something went wrong.
|
@lysnikolaou, your example helped. I've added a regression test. |
Sorry, something went wrong.
|
test_capi seems to be failing the first time, but it succeeds when re-run. Not sure what's wrong. |
Sorry, something went wrong.
|
Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
…Ready() (pythongh-105122) When I added the relevant condition to type_ready_set_bases() in pythongh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
|
GH-105211 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
…_Ready() (gh-105122) (gh-105211) When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
| cls->tp_base = (PyTypeObject *)Py_NewRef(base); | ||
| cls->tp_bases = Py_BuildValue("(O)", base); | ||
| if (cls->tp_bases == NULL) { | ||
| return NULL; |
There was a problem hiding this comment.
Should decref tp_base and tp_bases before returning.
Sorry, something went wrong.
There was a problem hiding this comment.
Opened gh-105225.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for following up after-the-fact!
Sorry, something went wrong.
| @classmethod | ||
| def setUpClass(cls): | ||
| # The tests here don't play nice with our approach to refleak | ||
| # detection, so we bail out in that case. | ||
| if cls._has_run: | ||
| raise unittest.SkipTest('these tests do not support re-running') | ||
| cls._has_run = True |
There was a problem hiding this comment.
This is nice. IMO, we should use a similar approach in test_import instead of the hack suggested in gh-104796.
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps also gh-105085 should have been solved like this.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm glad you appreciate the solution 😄, but actually am a bit torn about this. On the one hand, it gets us what we want in a simple, clear way (IMHO). On the other hand, this approach means there may be code paths that don't get checked for ref leaks, which we'd want to avoid.
If we use this pattern elsewhere, we'll need to be vigilant about making sure we don't miss any ref leak coverage. I'm not sure how easy that would be generally, though it's unlikely that there are more than a handful of tests where we'd want to skip refleak detection. FWIW, in this case I don't think ref leaks are much of a concern.
Sorry, something went wrong.
There was a problem hiding this comment.
we should use a similar approach
Wouldn't this skip the whole class Test instead of a smaller def test_...?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, so you make sure to collect all your "ref leak unsafe" tests in a single unittest class.
Sorry, something went wrong.
There was a problem hiding this comment.
Alternatively, create a decorator for it.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types.
We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific.