BUG: fix refcount issue caused by #12524 #12544
Conversation
| PyObject *seq = PySequence_Fast(*out_kwd_obj, "Could not convert object to sequence"); | ||
| PyObject *seq; | ||
| int ret; | ||
| Py_INCREF(*out_kwd_obj); |
There was a problem hiding this comment.
I don't think this is necessary, and it think it leaks if seq == NULL.
I think you should:
- Remove this line
- Add
*out_kwd_obj = NULLin the error path below - Replace
Py_SETREF(*out_kwd_obj, seq);with*out_kwd_obj = seq
There was a problem hiding this comment.
If the calling convention for PyUFuncOverride_GetOutObjects is that *out_kwd_obj is a borrowed reference, who will decref seq?
There was a problem hiding this comment.
Fixed the missing decref if seq == NULL
There was a problem hiding this comment.
Ahh, nvrmind. I didn't read your comment correctly. Fixing as per your reccomendation
There was a problem hiding this comment.
nit: you can now reorder these to eliminate the ret variable.
numpy/core/src/multiarray/methods.c
Outdated
There was a problem hiding this comment.
Not needed, if it errors then it produced no new reference
numpy/core/src/multiarray/methods.c
Outdated
There was a problem hiding this comment.
Can never be null here, so plain DECREF is fine.
Edit: This becomes true if you return None rather than NULL
There was a problem hiding this comment.
It seems simpler to me to return NULL in out_kwd_obj. Is there a style guide ref for things like that?
There was a problem hiding this comment.
I have no reference to give here, but I've might exist. One argument I can give in favor is that it allows you to eliminate the branching that happens inside XDECREF.
I might need to remind myself tomorrow about why this argument exists in the first place though.
There was a problem hiding this comment.
why this argument exists in the first place
We need it to hang on to a reference to the result of PySequence_Fast.
There was a problem hiding this comment.
I think to make refcounting easier, it would be tidier to return a new reference to Py_None here
|
Be good to finish this up, I'd like to release 1.12.0rc1 tomorrow. |
|
seems a little late for me to release 1.12.0rc1 ;) |
| seq = PySequence_Fast(*out_kwd_obj, | ||
| "Could not convert object to sequence"); | ||
| if (seq == NULL) { | ||
| return -1; |
There was a problem hiding this comment.
Should probably set *out_kwd_obj = NULL here just to be safe.
|
Restart tests. |
|
Thanks Matti, Eric. |
PR #12524 introduced a refcount issue. Changing the interface to
PyUFuncOverride_GetOutObjectsso that*out_kwd_objis never a borrowed reference should fix it.