zframe updates and minor typo fixes

[claws]

I really like your work to expose the CZMQ library to Python.
I was going to do this but you beat me to it - and I'm glad because its much better than I would have done.
Added zframe changes and a zframe test in line with czmq test.
Also added minor updates to types.py and fixed some typos.
The main motivation for these changes was to get access to non-string zframe.data buffer data. As well as sending standard string data I typically send serialised and compressed (binary) data in many of my ZMQ applications which gets corrupted with the assumption that zframe.data should always return a string.

[michelp]

Hi claws,

Thanks for all the cleanups and tests, I'd like to merge this PR but I'm curious what your rational was for removing the calls to ffi.gc for types like ctx and socket, was there a problem you encountered with the garbage collection function? I understand the ownership issues with cffi can be tricky so I've not doubt I may be doing this wrong.

[michelp]

BTW sorry for the delay, my github config was sending emails to the wrong address. Also thanks for returning types.py, as you could tell I ditched it as I wasn't certain I wanted to go down the road of maintaining an abstraction, but your work looks great and I would love to include it!

[michelp]

I don't see _frame used anywhere else?

[claws]

yep, that is an error left over from some earlier work.
I'm cleaning it up and will pushing another tested update soon.

[claws]

I have updated my fork with more tests and cleaned up the types a bit. The tests now all pass though the test of the Loop() type is nobbled as it is triggering a segfault. Something to do when czmq is trying to call the callback function. The tests/test_zloop.py works fine so it is some discrepancy with the Loop() implementation.

I was getting erratic behaviour with the ffi.gc calls. I believe the problem I encountered with the ffi.gc approach was double free'ing causing segfaults. I believe this was caused by me explicitly calling the .destroy() function. When implementing the czmq representative tests it is often necessary to manually destroy and object. Then when it finally gets garbage collected the destroy is called again which causes the segfault issue. I was encountering this a lot when adding the tests.

So I think there is two possible approaches; do the automatic destroy as well as preventing access to the destroy function OR leave it to the user to explicitly call destroy. As it seems likely that one can call an objects destroy function in typical applications (and this approach is used in the czmq representative tests) then the explicit approach seems like the way to go - hence the removal of the ffi.gc call in the new() functions.

[michelp]

Awesome thanks for the work, zloop was giving me segfaulting trouble too so don't sweat it that it doesn't work yet, I'll do some digging with gdb to try and figure out what going on.

While I'm usually a big fan of explicit is better than implicit, in this case I think Python programmers will be totally unaccustomed to garbage collecting their own resources. The double free in tests was only being causes because you were explicitly calling destroy on those wrapped objects. It's not necessary to call destroy on them in the tests when they are wrapped by ffi.gc, they will get destroyed when the test terminates and they go out of scope.

In the case of zmsg and zframe, the wrapping isn't possible, because those objects are destroyed when czmq sends them, but in the cases where the sending fails, is aborted, or just doesn't happen, there needs to be explicit collection, no getting around that, so those objects can't be wrapped.

So I think a good compromise is to ffi.gc wrap everything except zmsg and zframe and remove all the explicit calls to destroy in the tests. This was how I originally had it and there were no problems. If you want to explicitly test destroy, then create an unwrapped object with the low level binding.

[claws]

I agree that the auto gc is nicer.

However in tests like zctx and zauth, where I try to replicate the czmq tests, there is a need to explicitly delete the object. For instance in test_zauth the auth object is deleted to test that all connections are accepted after that event. As soon as I reinstate the ffi.gc I'm back to double free issues. Now, it could be argued that this is pretty unusual behaviour in real applications, once you add authentication it seems unlikely that you would turn it off.
So I could just remove this block from the test or add some sugar (an unwrapped new alternative?) to support it.

This is where the higher level types.py abstraction appeals to me. I kind of see two API's into the pyczmq library. One is to directly access low-level czmq functionality (explicitly) and another (perhaps the preferred) is the high-level (types.py) abstraction which is a more Pythonic style that would manage and hide much of the lower-level C API.

Thoughts?

[michelp]

Thanks for the explanation, it's good to get these different layers of abstraction nailed down early so we can avoid crossing layers later on. I want to reiterate that I intend to merge this PR once we work this issue out.

Like you I see layers, but 3 instead of 2, first, the low level "binding" layer, like pyczmq.C.zctx_new, etc. Next the higher level "functional" wrapper around those, that maps CFFI concepts as python (ffi.NULL becomes None, ffi.buffer and ffi.string are used) which have names like pyczmq.zctx.new. It's at this layer that I think auto gc would still be very useful. And then the third layer is of course the type abstractions that you've done such great work on which we both agree should hide the gc issues completely.

For the cases you mentioned, zctx and zauth, I haven't tried this, but it seems you can still use the gc wrapper and avoid the double free problem by explicitly deleting their references with 'del' and then asserting whatever further application state you want ("all connections are accepted after that event").

[claws]

I've added back the ffi.gc calls and used to 'del' to trigger the cleanup as suggested. The tests all work (except the nobbled test_loop in tests/test_types.py - which remains to be investigated) and I've added a zcert test.

[michelp]

Awesome! Great changeset, I'll merge and release