| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@rmadsen-ks you can look at callbacktest.cs and test_callback.py for testing this. |
Sorry, something went wrong.
|
@rmadsen-ks do you agree to transition from Zope to MIT license? |
Sorry, something went wrong.
|
@denfromufa, Test passed. I have added a test verifying that the fix does what it should. I agree to transition to MIT license. |
Sorry, something went wrong.
| import six | ||
| from Python.Test import ClassTest | ||
| from System.Collections import Hashtable | ||
| from System.Collections import Hashtable |
There was a problem hiding this comment.
where is this needed?
Sorry, something went wrong.
There was a problem hiding this comment.
It was not needed by me, but it seems I had added a windows line-ending. Its fixed now.
Sorry, something went wrong.
|
how about garbage collection, is CLR or Python responsible? If Python, then I do not see Decref, so it would leak memory? |
Sorry, something went wrong.
|
I have tested callbacks rapidly and the ref count does not increase permanently. Neither does it seem to leak when many new objects are made. I have tried creating 50M objects with no noticable leak. I should not need to Decref for the following reason: As the comment in pyobject.cs says, the PyObject assumes ownership. So if we want to convert a python object to a PyObject using ToManagedValue, we must increase the refcount when we are giving it to the constructor of PyObject. So I implemented that part. The PyObject decreases the ref count when the garbage collector cleans it up, which happens eventually. So when a new C# PyObject has been created from a python object, it will eventually be cleaned up by the .NET GC. Is some cases using(){} can also be used to give more predictable memory performance. In the same way, if we want to convert C# PyObject to a python object, we increase the ref count, because now Python has an extra reference to it, so it needs ownership. Python will clean that up on its own when it goes out of scope. |
Sorry, something went wrong.
|
@tonyroberts @vmuriart @filmor please review! |
Sorry, something went wrong.
|
|
||
| //========================================================================== | ||
| // Tests calling from Python into C# and back into Python using a PyObject. | ||
| // SelfCallbackTest should be inherited from a Python class. |
There was a problem hiding this comment.
Very minor nitpick, but shouldn't that read "inherited by a Python class"?
Sorry, something went wrong.
There was a problem hiding this comment.
Comment has been changed.
Sorry, something went wrong.
|
@rmadsen-ks this pull request does NOT resolve the issue described in #94: dta@dta-Inspiron-N5050 ~/Downloads $ sudo pip install pythonnet-fix-develop-296-convert-PyObject.zip -U --force
Processing ./pythonnet-fix-develop-296-convert-PyObject.zip
Installing collected packages: pythonnet
Found existing installation: pythonnet 2.2.0
Uninstalling pythonnet-2.2.0:
Successfully uninstalled pythonnet-2.2.0
Running setup.py install for pythonnet ... done
Successfully installed pythonnet-2.2.0
You are using pip version 8.1.1, however version 9.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
dta@dta-Inspiron-N5050 ~/Downloads $ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import clr
>>> from System import Object, Array
>>> import sys
>>> a = Array[Object]([sys]) ; print a[0]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: value cannot be converted to Object
|
Sorry, something went wrong.
|
Hi, I'm not suggesting that it does. You want #94 fixed as part of this? |
Sorry, something went wrong.
|
@denfromufa, is there anything I need to do to move this further or do I need to find another solution? |
Sorry, something went wrong.
|
@rmadsen-ks your pull request is completely valid. |
Sorry, something went wrong.
|
To me this also looks fine, but why do you introduce a new RefCount property? Either remove it or add tests for it, after that we can merge. |
Sorry, something went wrong.
|
@filmor, I found it useful for verifying that I was not leaking reference counts. Without it, it is hard to get out the reference cound from C# as the NativeMethods are internal. It is not critical for me, so we can remove it. |
Sorry, something went wrong.
|
@rmadsen-ks can you please open a new PR? I made a mistake and merged this pull request without seeing @filmor concern. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Work in progress.
Note, testing has not been done, except for exploratory style. How is testing supposed to be done?