allow for deletion of handles from the map in gopyh.

[cosnicolaou]

This PR addresses #210 and #217. It adds the ability to delete the handles managed by the gopyh package. To do so, it reference counts the use of each handle and deletes handles when that count goes to zero. In addition, deletion of handles exposed the problem identified by #217 where integer values can be (sometimes incorrectly) treated as handles. The adopted fix is to special case assignment of basic types vs gopy managed objects in struct setters as per the comments in bind/gen_struct.go.

Fixes #217.

[rcoreilly]

Overall looks reasonable to me. Basically the Python wrapper objects now have a __del__ destructor and that de-registers their handle -- seems sensible. By deleting from the handles map, it allows, but does not compel, the Go GC to delete the object on the Go side, if it is not otherwise being used. Assuming your tests all check out, then I can't seen any obvious problems with this approach!

I should have thought to look for a Python destructor function but really my detailed experience with Python has mainly been in this project :)

So is there some further Pythonic way we might want to protect access to the handles so they can't be incorrectly used, potentially after deletion? I might have missed it but I don't think I saw that. And I can't remember now to what extent any of the generated code might use other object's handles -- that would need to be fixed if so.

[cosnicolaou]

I'm not a python expert either:-) Do you know why the API even allows handles to be passed directly to the constructor? The only use I can imagine is to allow multiple python objects to point to the same go object, maybe to share go objects across modules? If I knew the original use case I may be able to come up with a better solution.

[rcoreilly]

Maybe if you access a struct-element from a slice, for example, it creates a new python object and passes the handle as the constructor? I really have been away from this code long enough that I forget how it works!

[sbinet]

I don't understand this change.
originally, this was testing type-correctness.

[cosnicolaou]

this PR deletes handles from the gopyh handle's table, so constructing an object from a deleted handle will fail. The code as it stood took an argument of 1, which inside the constructor is interpreted as a handle by
the assignment:

self.P1 = args[0]

Since the value is 1, and that handle was deleted (when the original hi.Anon() value was gc'ed by python), this will fail by indirecting through a nil pointer. (The error handling could be improved obviously).

My change to the test ensures that it always uses a valid handle.

Also, the test code itself is confusing in that it says '*ERROR exception not raised' etc but the main_test expected data expects the the exception to not be raised...

*ERROR* no exception raised! *ERROR* no exception raised! *ERROR* no exception raised! --- testing GC...

Let me know how to proceed with this. I now realise that I need to refcnt the handles too, I'll add that shortly.

[sbinet]

well... this must be some uncaught regression.
(I haven't bisected but I suspect that's since the migration to pybindgen)
it used to be that invalid arguments would be caught and an exception be raised.
(I agree, printing *ERROR* isn't a very robust mechanism...)

[sbinet]

so, to circle back on this.
I'd rather have the original test being commented out (with the hi.Couple(1) left as is) instead of changing the purpose of the test.

I have filed #217 so we don't forget about this.

[cosnicolaou]

yep, that's my plan too. I looked back through the git history and can see what happened, there's a TODO in the code. I'll hopefully get this cleaned up today or tomorrow.

[cosnicolaou]

ok, please take another look, especially at the comments in gen_struct. I think those changes make sense, but the underlying problem is that the confusion between int values that are handles and those that are not and I don't see any clean/easy way of resolving that beyond what I've done here. Let me know if you have any other ideas.

[cosnicolaou]

I see the uses of handle= in the generated code that rcoreilly alludes to. I'll add refcounting to allow for that.

[sbinet]

looking good! thanks.

perhaps mention #217 in a commit? (so we can close it when merging this PR in)

[cosnicolaou]

all done, thanks!