-
Notifications
You must be signed in to change notification settings - Fork 748
Fixed #296 Converter support for PyObject #297
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
Fixed #296 Converter support for PyObject #297
Conversation
@rmadsen-ks you can look at callbacktest.cs and test_callback.py for testing this. |
@rmadsen-ks do you agree to transition from Zope to MIT license? |
@denfromufa, Test passed. I have added a test verifying that the fix does what it should. I agree to transition to MIT license. |
@@ -6,7 +6,7 @@ | |||
import System | |||
import six | |||
from Python.Test import ClassTest | |||
from System.Collections import Hashtable | |||
from System.Collections import Hashtable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not needed by me, but it seems I had added a windows line-ending. Its fixed now.
how about garbage collection, is CLR or Python responsible? If Python, then I do not see Decref, so it would leak memory? |
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 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. |
@tonyroberts @vmuriart @filmor please review! |
|
||
//========================================================================== | ||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor nitpick, but shouldn't that read "inherited by a Python class"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment has been changed.
@rmadsen-ks this pull request does NOT resolve the issue described in #94:
|
Hi, I'm not suggesting that it does. You want #94 fixed as part of this? |
@denfromufa, is there anything I need to do to move this further or do I need to find another solution? |
@rmadsen-ks your pull request is completely valid. |
To me this also looks fine, but why do you introduce a new |
@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. |
@rmadsen-ks can you please open a new PR? I made a mistake and merged this pull request without seeing @filmor concern. |
It is here #306 |
Work in progress.
Note, testing has not been done, except for exploratory style. How is testing supposed to be done?