Remote refcount fix

[mduggan]

Fix the refcounts in remotes (issue #403) by setting the callbacks just before fetch, and resetting to defaults after. I think I got it right, but I'm not super-familiar with cffi so let me know if I did something stupid.

I also added a few tests to look for refcount leaks after accessing various properties.

[carlosmn]

There's no need for this line. You're already setting the version above with the init function. If the version does increase, this would cause libgit2 to consider the struct to have a different layout.

[mduggan]

Ah, I was doing it in a little bit of a weird order.. I wanted to create the "default" callbacks first so that any error could raise before the custom callbacks had been set.. but fair point about the version number being set differently on the two lines.. if it changes in one place it will need to change in both,

[jdavid]

Okey, there is just travis complaining with pypy, can you look at this?

[mduggan]

Sorry for the slow reply, I'm a little bit confused by what's going on with pypy but I will fix it - I've just been away for a few days and will look at this tomorrow.

[carlosmn]

The issue is the different GC algos each use. CPython is keeping the handle alive much longer than pypy is (or pypy is moving the object, but that's less likely here, I think).

When you assign callbacks.payload = ffi.handle(self), the python object immediately leaves the scope of python objects and is free to be reclaimed, which pypy does in this situation, making the payload/data handle an invalid pointer, which looks like it's what pypy tries to tell you.

This is the reason why the code in master assigns self to a pointer of itself, so callbacks.payload does not become unreferenced. Depending on how paranoid we want to be about GC, we have to assign the handle first to a variable, or attach it to self itself (temporarily). Doing

-        callbacks.payload = ffi.new_handle(self)
+        self_handle = ffi.new_handle(self)
+        callbacks.payload = self_handle

is enough for it to pass right now. I'm not sure if there's any rules about python variables being kept alive until the end of the function or not. If not, we might want to do

self._self_handle = ffi.new_handle(self)
callbacks.payload = self._self_handle
... # fetch and stuff
del self._self_handle

to make sure that the handle doesn't become unreferenced on the python side for as long as the callbacks struct is alive.

[mduggan]

Oh right, thanks for the explanation! I failed to realise that because callbacks doesn't behave exactly like a Python object, pypy doesn't keep the members alive for me. It's probably right to be a bit paranoid too - even though self_handle would stay in scope, if there's no reference to it then Pypy may be within its rights to clean it up in the absence of a with.

[carlosmn]

Yeah, it's a similar issue as with a git_strarray. We also need to keep the references around as even CPython will remove them (presumably because it's across a function return).