Burgestrand/Hallon — Issue #61: Minor memory bloat related to callbacks
Author: Burgestrand
Created: 2011-12-15 22:32:12+00:00
State: open
Labels: Investigate, Refactor

As of currently, callbacks are attached to their associated pointer. There is this complicated mechanism with ObjectSpace.define_finalizer for the callback structs, but it’s not really needed now as the callbacks can only be garbage collected once their pointer is garbage collected anyway (which means that the callback structs are also removed automatically).

There’s a big issue here with callbacks due to the nature of how asynchronous callbacks from non-ruby threads must be handled ([also see one of my blog posts on how it’s done](http://burgestrand.se/articles/asynchronous-callbacks-in-ruby-c-extensions.html)). I have some documentation of this inside `spotify.rb` as well.

The issue is this:

- (thread #1) creating image pointer
- (thread #1) adding callbacks to image pointer
- (thread #3) background thread decides image is loaded, calls callback
- (thread #3) ffi receives callback, waits to get ruby GIL
- (thread #1) removes callbacks from image
- (thread #1) garbage collects image, libspotify removes image from memory
- (thread #3) ffi receives GIL, calls callback over to ruby
- (thread #2) receives image loaded callback (with invalid image pointer, as it is garbage collected)
- (thread #2) tries to call image_loaded_callback (that is now GC’d), ruby crashes
… now, assuming image_loaded_callback does exist (even though it wont) and ruby does not crash
- (thread #2) image_loaded_callback returns
- (thread #3) ffi returns callback return value
- (thread #3) image loaded callback returns in spotify background thread… WHAT NOW? image is gone anyway!

Essentially, for any callback that’s called asynchronously (and not through, say, session_process_events), *ANY* registered callback proc must *NEVER* be garbage collected. Ever. Because you could always have a callback in the background thread just waiting to execute once the main thread runs out of work to do, and then I’m royally fucked.

Now, I have a theory. The libspotify documentation has few functions (4? all related to audio) that’s explicitly documented to be called from “an internal callback thread”. If these are the only ones, the current approach is possibly good enough if I make the 4 asynchronous callbacks constant and non-changing. Not too bad a compromise.

If not, there’s a chance Hallon will crash with strange memory errors of calling call or trigger on Ruby::VM or pretty much any object that’s unlucky enough to end up in the spot the now garbage-collected callback proc used to reside in. I‘ll leave this issue open for a while.

See #60 and ed0016aa77b3f54c8c01cd690b512fd4f3aba06d — both are very relevant.

(PS: one could use constant callbacks for everything, and use some sort of finalizer logic to call the callback on the proper objects from the callback [assuming the objects register themselves as interested], but that’s for another day)