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] Reference count leak with memory view arguments #4296

Closed
peterbell10 opened this issue Jul 19, 2021 · 1 comment · Fixed by #4476
Closed

[BUG] Reference count leak with memory view arguments #4296

peterbell10 opened this issue Jul 19, 2021 · 1 comment · Fixed by #4476

Comments

@peterbell10
Copy link

Describe the bug
When a function has multiple memory view arguments, and the argument call fails because of a type-mismatch then it can leak references to original array being viewed.

To Reproduce
In memleak.pyx:

def foo(double[:] a, double[:] b):
    pass

and in repro.py:

import sys
import numpy as np
from memleak import foo

a = np.random.rand(100)
b = np.random.randint(100)

for i in range(1000):
    try:
        foo(a, b)
    except:
        pass

    if i % 100 == 0:
        print(sys.getrefcount(a), sys.getrefcount(b))

foo is called with a valid array for a but an invalid b. The result is a's refcount is constantly increasing:

4 12
204 12
404 12
604 12
804 12
1004 12
1204 12
1404 12
1604 12
1804 12

Expected behavior
The refcount of a should be constant, like it is for b.

Environment (please complete the following information):

  • OS: Linux
  • Python version 3.8.8
  • Cython version 0.29.23
@da-woods
Copy link
Contributor

I think the problem is that the cleanup:

__PYX_XCLEAR_MEMVIEW(&__pyx_v_a, 1);
__PYX_XCLEAR_MEMVIEW(&__pyx_v_b, 1);

is inside the implementation function foo rather than the wrapper function. Since it fails in the wrapper function and doesn't get to the implementation function then the leak occurs.

It has the signs of "might be fiddly to fix". I think it only applies to memoryviews, but in principle could affect any ref-counted, non-pyobject type added in future

da-woods added a commit to da-woods/cython that referenced this issue Nov 21, 2021
Fixes cython#4296

If there was an error in preparing the function arguments after a
memoryview had already been created then the memoryview was not
cleaned up correctly.

(This leaves it in the slightly odd position where memoryviews
are cleaned up in the wrapper function on failure, but in the
main function on success. I kind of think it'd be better to clean
them up in the wrapper function in both cases, but I'm reluctant
to mess with a system that largely works)

I doubt this can be backported cleanly to 0.29.x so may have to
stay at a 3.0 bug-fix for now
scoder pushed a commit that referenced this issue Nov 24, 2021
Fixes #4296

If there was an error in preparing the function arguments after a
memoryview had already been created, then the memoryview was not
cleaned up correctly.

(This leaves it in the slightly odd position where memoryviews
are cleaned up in the wrapper function on failure, but in the
main function on success. I kind of think it'd be better to clean
them up in the wrapper function in both cases, but I'm reluctant
to mess with a system that largely works.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants