search

Burgestrand / Hallon

Hallon is currently **UNMAINTAINED**.
http://rdoc.info/github/Burgestrand/Hallon/master/frames
135 stars 12 forks source link

Refactor the way pointers are handled #42

Closed Burgestrand closed 12 years ago

Burgestrand commented 12 years ago

A proper solution to this problem would hopefully solve #41 as well.

Currently, pretty much all Hallon objects accept an FFI::Pointer as an argument. Problem with this is that Hallon cannot know if this pointer needs its’ reference to be upped or not, but tries to make an educated guess. Most of the time this works, but it’s unreliable and frankly it makes the code harder to grasp.

In addition, Hallon also wraps this pointer into a Spotify::Pointer. Even at this point, we do not know if this pointer has an added reference. Still, this is the only object we care about existing in the first place.

Now, assuming we convert fully into using only AutoPointers (and such, Spotify::Pointer), we need some basic assumptions:

  1. If we have a Spotify::Pointer, its’ GC is fully taken care of (and will be released when it’s GC’d by ruby).
  2. If we don’t have a Spotify::Pointer, we can wrap an existing pointer and make it one.
  3. Wrapping an existing pointer into a Spotify::Pointer means we extend the life of the underlying pointer until the Spotify::Pointer is released by ruby (at which point we free it).

This spawns the question, who is responsible for giving life to the underlying possible with add_ref, if necessary? Is it the creator, or the wrapper? Thinking of it for a while makes it pretty clear that the wrapper object itself should be self-managing, and if it’s given a pointer that already has its’ refcount at 1 instead of 0, it’s the callers’ duty to decrease the refcount by one after wrapping the pointer.

As a final crazy idea, I had this thought that I could monkeypatch/wrap/proxy the Spotify API, so that the pointers it returns always has their refcount set at 0. That would make this a non-issue for me, but it would make it harder to use the raw API in conjunction with Hallon, and I think it’s already hard enough, tyvm.

Top priority on this at the moment. Creating the issue so I remember my thought process tomorrow morning when I’m sane enough to judge my own ideas (and not subject poor Elin to them).

Burgestrand commented 12 years ago

Further zen: there should never circulate raw FFI::Pointers when using Hallon.

Burgestrand commented 12 years ago

As a final crazy idea, I had this thought that I could monkeypatch/wrap/proxy the Spotify API, so that the pointers it returns always has their refcount set at 0. That would make this a non-issue for me, but it would make it harder to use the raw API in conjunction with Hallon, and I think it’s already hard enough, tyvm.

Went with this one, in a sense. For each function in Spotify that returns a new object, I’ve created a new function with the same name except it ends with a bang. This new function returns a Spotify::Pointer, that always has the correct reference count set on it.

As I didn’t modify existing behaviour, I’m okay with the impact of this change. All that’s left is the final (big) refactor to using this API instead in Hallon.

Burgestrand commented 12 years ago

Only Search (radio and normal), AlbumBrowse, ArtistBrowse and Toplist left now! (I… think).

Burgestrand commented 12 years ago

With all this, I believe we’re done. rake spotify:coverage will show warnings if non-auto-GC methods are being used.