GeezerVison is showing github.com — Memory Leak on MethodBinding for generic method · Issue #691 · pythonnet/pythonnet
Memory Leak on MethodBinding for generic method #691DescriptionEnvironment
- 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 1I 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 Activitywilliamsardar commented on Jun 21, 2018 ContributorAuthor More actions I am happy to do a fix for this - would that be the fastest way to get this fixed? den-run-ai commented on Jun 21, 2018 Reacted by William Sardar williamsardar commented on Jul 4, 2018 ContributorAuthor More actions 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] # leakWhen 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 den-run-ai commented on Jul 5, 2018 @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:
|
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.
williamsardar commented on Jul 5, 2018 ContributorAuthor More actions @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? williamsardar commented on Jul 23, 2018 ContributorAuthor More actions 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 filmor commented on Jul 23, 2018 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? williamsardar commented on Jul 24, 2018 ContributorAuthor More actions 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? den-run-ai commented on Jul 24, 2018 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.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|