Memory Leak on MethodBinding for generic method

Environment

Pythonnet version: 2.3.0
Python version: 3.6-32
Operating System: Windows 10

Details

Getting a MethodBinding for a generic method results in the reference count being too high, so the tp_dealloc method on the MethodBinding never gets called, even when the MethodBinding goes out of scope.

namespace PlainOldNamespace
{
    public class PlainOldClass
    {
        public void NonGenericMethod() { }

        public void GenericMethod<T>() { }
    }
}

import clr, sys
clr.AddReference("PlainOldAssembly")
from PlainOldNamespace import PlainOldClass
poc = PlainOldClass()
sys.getrefcount(poc.NonGenericMethod)
# 1
sys.getrefcount(poc.GenericMethod[clr.System.Double])
# 2 - should be 1

I think this could be down to a bug in MethodBinding.cs. This line looks suspicious. Why should the reference count get incremented here?
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/methodbinding.cs#L59

I am happy to do a fix for this - would that be the fastest way to get this fixed?

@williamsardar

@williamsardar I'm not sure yet about this problem, can you provide a unit test where something fails because of this? There are 2 places like this in the codebase:

https://github.com/pythonnet/pythonnet/search?q=Runtime.XIncref%28mb.pyHandle%29&unscoped_q=Runtime.XIncref%28mb.pyHandle%29

There don't appear to be any unit tests that will show this problem. I've seen there is the leaktest.py file. Not clear that this code gets run on the CI server as it contains syntax errors. Using the approach in leaktest.py (create and del an object 50,000 times), and adding a 1MB allocation to the PlainOldClass type shows that I get an OutOfMemory exception before making the fix, and don't get one after making the fix. Please could you let me know how likely it is to get this fix incorporated and a new version released? Happy to help where I can..

namespace PlainOldNamespace
{
    public class PlainOldClass
    {
        private readonly byte[] payload = new byte[(int)Math.Pow(2, 20)]; //1 MB
        public void GenericMethod<T>() { }
    }
}

import clr, sys
clr.AddReference("PlainOldAssembly")
from PlainOldNamespace import PlainOldClass

for i in range(50000):
    PlainOldClass() # no leak

for i in range(50000):
    PlainOldClass().GenericMethod # no leak

for i in range(50000):
    PlainOldClass().GenericMethod[str] # leak

When I comment out the suspicious line, the OutOfMemory exception does not happen. All python and .Net unit tests pass with this fix.

https://github.com/pythonnet/pythonnet/blob/master/src/runtime/methodbinding.cs#L59

@williamsardar

@williamsardar you are correct that leaktest.py is not running as part of CI. It may have been lost during transition between 3 hosting providers in the past 15 years of this project and last time I found it running was 2008 with .NET 2.0:

https://mail.python.org/pipermail/pythondotnet/2008-January/000771.html

Anyway, here are some points to consider before making a pull request:

Like I mentioned before there are 2 places like this in the pythonnet, but there is also a third one where this ref-count incrementing was commented out with some comments:

pythonnet/src/runtime/constructorbinding.cs

Line 109 in 54841da

Runtime.XIncref(boundCtor.pyHandle); // Decref'd by the interpreter??? */

So it might be worth agreeing on all 3 cases as part of this PR?

Optionally you can include your code in leaktest.py and run with the tests, then disable anything that is currently leaking/crashing.
If the test is taking too long to run, maybe it should only run on latest Python version, e.g. Python 3.6.

@denfromufa It would clearly be desirable to have some kind of memory leak monitoring running as part of CI. We are using PythonNet to give Python clients access to our .Net data retrieval library that returns large double arrays. Typical use cases involve looping over many data sets, a classic memory leak sensitive scenario. (our .Net library actually calls into a low-level C++ library using SWIG, which has posed its own memory management challenges).

I am not sure the leak tests are suitable to run as unit tests "as-is" because they do not give a conclusive binary result. The approach is to compare the allocated memory before and after looping over many "memory-light" operations, the resulting memory delta should be zero. However, according to the code comments in leaktest.py, the delta will not always be zero, even in a success case (slightly suspicious in itself). Since these are "memory-light" operations, it's not easy to distinguish between expected small allocation increase due to normal framework operations, and unexpected small allocation increase due to leakage. It is be better to use "memory-heavy" operations so the delta in a leak-case would be much more obvious.

Clearly, the technique I've used to detect memory leaks (keep looping until an OutOfMemory exception is thrown) does have a binary result - either an exception is thrown or it's not. However, please note I am running a 32-bit python client process due to legacy constraints on my .Net library. This means the out of memory happens at about 1.6GB allocated. The same approach with a 64-bit process could cause system instability as the process memory space is practically unlimited. Further, it's risky triggering an out of memory scenario as this could also bring down the unit test framework which shares the same process memory space. It might be possible to write a test that calls an operation that allocates 1MB, say, 500 times. If, after garbage collection on both the python and .Net end, the allocation has increased by more than, say 400 MB, then we know something has gone wrong. This could allow memory leak detection without OutOfMemory. Does this sound sensible?

I don't have a lot of time to contribute a leak test suite (this is a commercial project), but should be able to contribute something. I am a .Net developer quickly learning enough about the world of Python to get by. What is the mechanism by which the test runner in the CI picks up the test files (e.g. test_generic.py)? And what is it about leaktest.py that means it's not currently being run?

Hi @denfromufa
I've made a pull request that fixes the problem, and related problems, and adds unit tests to show this.
Once it's been merged, can you say how likely it would be that a release could be made in the near future? This memory leak will stop us shipping our product which makes heavy use of generic methods.
Thank you!
William

I'd like to land a few more of the very long abandoned PRs before doing an official release. However, building is not really difficult, if this is keeping you from shipping, would it be an option to just take the current master instead of waiting?

We would like our users to get pythonnet from PyPI. I'm not that familiar with the world of Python - is this possible without an official release?

You can just run installation from master (pip+git) as part of your app setup: https://github.com/pythonnet/pythonnet/wiki/Installation But pythonnet is just 2 files which you can also include in your app. There is one release blocking issue + python 3.7 fix required before next version of pythonnet can be pushed to pypi.

…

On Tue, Jul 24, 2018, 11:41 AM William Sardar ***@***.***> wrote: We would like our users to get pythonnet from PyPI. I'm not that familiar with the world of Python - is this possible without an official release? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5dJbzahowR1t3tGsWJr0O7Uve3Rbks5uJsGCgaJpZM4Ur1gi> .

Fix MethodBinding/OverloadMapper memory leak (pythonnet#691)

Fix MethodBinding/OverloadMapper memory leak (#691) (#2719)

Skip leaky overloaded-method binding memory test

Skip last leaky method-overloads binding memory test

Fix net10 CI: workflows, generic re-registration, conversions (#115)

Fix MethodBinding/OverloadMapper memory leak (pythonnet#691) (pythonn…

Navigation Menu

Repository navigation

Memory Leak on MethodBinding for generic method #691

Environment

Details

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Memory Leak on MethodBinding for generic method #691

Description

Environment

Details

Activity

williamsardar commented on Jun 21, 2018

den-run-ai commented on Jun 21, 2018

williamsardar commented on Jul 4, 2018

den-run-ai commented on Jul 5, 2018

williamsardar commented on Jul 5, 2018

williamsardar commented on Jul 23, 2018

filmor commented on Jul 23, 2018

williamsardar commented on Jul 24, 2018

den-run-ai commented on Jul 24, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions