Skip to content
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 reference count leak in ufunc dtype handling #19289

Merged
merged 2 commits into from Jun 24, 2021

Conversation

seberg
Copy link
Member

@seberg seberg commented Jun 21, 2021

This adds a missing decref to the signature/dtype keyword argument
logic in reductions.
(The code will change quite a bit after 1.21, but this avoids a
reference count leak on 1.21.)

EDIT: It may take until tonight for me to know whether there is much (or anything) more coming...

This adds a missing decref to the signature/dtype keyword argument
logic in reductions.
(The code will change quite a bit after 1.21, but this avoids a
reference count leak on 1.21.)
@seberg seberg changed the title BUG: Fix reference count leak BUG: Fix reference count leak in ufunc dtype handling Jun 21, 2021
@seberg
Copy link
Member Author

seberg commented Jun 23, 2021

OK, (hopefully) all of the others are either expected or false positives due to pytest fixtures I think.

@@ -4054,6 +4054,7 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc,
}
Py_INCREF(dtype->singleton);
otype = dtype->singleton;
Py_DECREF(dtype);
Copy link
Member

@charris charris Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of explanation would be good here. I assume that dtype already holds a reference to the singleton member and cleans that up when dtype is destroyed. True? Might read better like

       otype = dtype->singleton;
       Py_INCREF(otype);
       Py_DECREF(dtype);

Copy link
Member Author

@seberg seberg Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is better. This is a bit weird: singleton is guaranteed to exist and dtype can never be destroyed because this is limited to "old-style" DTypes.

This code is simply not yet new-style user DType ready. The whole dance will vanish (or rather: move into legacy-only fallback branches) with the ufunc refactor. Until then, this is a bit awkward because I try to introduce the new logic step-by-step.

`ensure_dtype_nbo()` already increments the reference count,
so the INCREF is not necessary here.
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jun 24, 2021
@charris charris added this to the 1.21.1 release milestone Jun 24, 2021
@charris charris merged commit 2b18f72 into numpy:main Jun 24, 2021
@charris
Copy link
Member

charris commented Jun 24, 2021

Thanks Sebastian.

@seberg seberg deleted the fix-refleaks branch June 24, 2021 21:27
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 25, 2021
@charris charris removed this from the 1.21.1 release milestone Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants