-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: Fix some multiarray leaks #19429
Conversation
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.
Thanks, looks good! Only that one one is fix is not quite at the right place.
@@ -1089,6 +1089,7 @@ PyArray_DiscoverDTypeAndShape_Recursive( | |||
} | |||
/* The cache takes ownership of the sequence here. */ | |||
if (npy_new_coercion_cache(obj, seq, 1, coercion_cache_tail_ptr, curr_dims) < 0) { | |||
Py_DECREF(seq); |
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 should be in npy_new_coercion_cache
(a function that steals a reference also steals the reference on error).
@@ -234,6 +234,7 @@ PyArray_CastScalarToCtype(PyObject *scalar, void *ctypeptr, | |||
descr = PyArray_DescrFromScalar(scalar); | |||
castfunc = PyArray_GetCastFunc(descr, outcode->type_num); | |||
if (castfunc == NULL) { | |||
Py_XDECREF(descr); |
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.
Actually, this check should be done immediately, calling PyArray_GetCastFunc
if descr
is NULL is not going to work out.
EDIT: So, this can be a DECREF
, but we need an if (descr == NULL)
immediately.
@@ -4908,5 +4908,6 @@ PyMODINIT_FUNC PyInit__multiarray_umath(void) { | |||
PyErr_SetString(PyExc_RuntimeError, | |||
"cannot load multiarray module."); | |||
} | |||
Py_XDECREF(m); |
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.
m
may be uninitialized if npy_cpu_init
fails. Maybe just put the module creation first?
@defoishugo gentle ping. It is perfectly fine if the PR stays around for a long time, but since these are not very big changes maybe you have time to wrap it up? |
close/reopen |
Looks like the CI upgrade to Python 3.8.11 breaks the tests. |
Just a bump in case someone has some time, I had fixed this up, so we should probably get it in. It is pretty simple. |
Thanks @defoishugo |
Resolves #19307.
Resolves #19287.
Resolves #19288.
This merge request only adds some Py_(X)DECREF calls, so the reference count will be always decresed.
The reported cases where about very specific cases. There is no big leak, but it's always good to improve the general memory management.