BUG: dtype refcount cleanups #14586
BUG: dtype refcount cleanups #14586
Conversation
| { | ||
| Py_DECREF(dtype); | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
this was one of the places that produced the error message, and was straightforward to find
There was a problem hiding this comment.
PyArray_DescrFromType looks weird, it returns a new reference for strings, returns a borrowed reference otherwise.
There was a problem hiding this comment.
That function basically knows that some dtypes are immutable, while flexible dtypes are sometimes "adapted" so they need to be copied...
There was a problem hiding this comment.
Wait, it returns a borrowed reference? I thought it always increfs, now I am confused :), have to look closer at it later...
There was a problem hiding this comment.
Ah, sorry. I keep confusing PyArray_NewFromDescr and this one (if you want to be clean, add the NULL check). Isn't it enough to replace this with NewFromDescr (which practically never fails, except for flexible types)? The cleanup should not be necessary, since FromString has to steal the reference.
| if (ret == NULL) | ||
| { | ||
| Py_DECREF(descr); | ||
| } |
There was a problem hiding this comment.
this fix came from reviewing the use of PyArray_DescrFrom* in the C file, and did not actually leak (yet).
There was a problem hiding this comment.
Wow, same bug yeah. Same thing, just make it PyArray_DescrNewFromType. With NULL check if you wish.
EDIT: To be clear, the ret == NULL check is not correct here.
numpy/core/src/multiarray/ctors.c
Outdated
| Py_DECREF(dtype); | ||
| if (r == NULL) { | ||
| Py_DECREF(dtype); | ||
| } |
There was a problem hiding this comment.
this was the source of the other error report, and is a bit subtle: if r is an ndarray we do not need to decref
There was a problem hiding this comment.
Looks like r === NULL case is already handled up above. What am I missing?
There was a problem hiding this comment.
The DECREF can be done unconditioanlly here, I agree. I do not think that path can be taken at all.
|
Reminder to self: use |
74f6f06 to
9ce555e
Compare
|
|
||
| string = PyBytes_AsString(byte_obj); | ||
| if (string == NULL) { | ||
| return NULL; | ||
| } | ||
|
|
||
| return PyArray_FromString( | ||
| string, -1, PyArray_DescrFromType(NPY_FLOAT64), -1, " "); |
There was a problem hiding this comment.
What's the premise behind splitting this line?
There was a problem hiding this comment.
FWIW, I prefer one function per line when debugging, it saves stepping in to the wrong function and my mind can parse it more easily. But no big deal either way. If this is a blocker I will reformat it.
There was a problem hiding this comment.
That's reasonable, especially since usually multiple python API calls on a line means you forgot to check for NULL.
| @@ -601,14 +601,14 @@ static PyObject * | |||
| fromstring_null_term_c_api(PyObject *dummy, PyObject *byte_obj) | |||
| { | |||
| char *string; | |||
| PyArray_Descr *descr; | |||
There was a problem hiding this comment.
May as well put this on line 610, unless the plan is to backport
|
The decref on line 3656 looks wrong to me too |
|
Heh... now had to look again. So I think, that aside from the tests, the actual error is this DECREF (the other DECREF should stay as it is), in that it has to be removed: numpy/numpy/core/src/multiarray/ctors.c Line 3794 in 88f0808
|
|
I think For simplicity, can we rewrite these internal helpers to not steal a reference? From what I've seen so far, places where references are stolen are inevitably the places where we clean up badly. |
I am fine with that, it seems like it does end up being to smart. To defend the old code a bit, I think all array creation functions steal the reference to the descritpor. |
|
OK, I ran refcount checker over |
| @@ -3793,7 +3786,6 @@ PyArray_FromFile(FILE *fp, PyArray_Descr *dtype, npy_intp num, char *sep) | |||
| (skip_separator) fromfile_skip_separator, NULL); | |||
| } | |||
| if (ret == NULL) { | |||
| Py_DECREF(dtype); | |||
There was a problem hiding this comment.
Just to be clear, this line is likely the only actual bug. Everything else is now just cleanup...
|
git bisect says this PR broke scipy.io.FortranFile, see scipy/scipy#10903, |
| return NULL; | ||
| } | ||
| /* In some cases NewFromDescr can replace the dtype, so fetch new one */ | ||
| dtype = PyArray_DESCR(r); |
There was a problem hiding this comment.
This I think breaks reading sub-array dtypes:
# After this PR
>>> import numpy as np
>>> with open('/dev/zero', 'rb') as f:
... a = np.fromfile(f, dtype='(8,8)u8', count=1)
...
>>> (a == 0).all()
False
The reason I guess is that creating an array with sub-array dtype moves the array dimensions from the dtype to the array itself, so the number of bytes to read becomes incorrect.
There was a problem hiding this comment.
Oh should have thought of that. Yeah, if I do it like that, I also have to update the num from the actual array...
gh-13605 did not handle refcounts of dtype instances. I discovered this by running
which printed the following twice at the end
The lack of those reference count errors when running all the tests (before this PR) would suggest we have places in the code that leak references by incref-ing without decref-ing.