-
-
Notifications
You must be signed in to change notification settings - Fork 393
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
Remote refcount fix #404
Remote refcount fix #404
Conversation
|
||
# Build the callback structure | ||
callbacks = ffi.new('git_remote_callbacks *') | ||
callbacks.version = 1 |
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.
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.
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.
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,
Okey, there is just travis complaining with pypy, can you look at this? |
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. |
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 This is the reason why the code in master assigns self to a pointer of itself, so - 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. |
Oh right, thanks for the explanation! I failed to realise that because |
Yeah, it's a similar issue as with a |
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.