Sporadic crash during iterative search queries #121

Closed
liamw9534 opened this Issue Mar 14, 2014 · 3 comments

Projects

None yet

2 participants

@liamw9534

Seeing the following crash in the python wrapper from an application I have written that is performing a number of iterative search queries:

From callback <function _search_complete_callback at 0x103ad70>:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/spotify/search.py", line 308, in _search_complete_callback
    search_result._callback_handles.remove(handle)
AttributeError: '_cffi_backend.CData' object has no attribute '_callback_handles'

I took a look at the code and couldn't see an obvious way by which the class could be initialized without the _callback_handles attribute being assigned i.e., it gets assigned to an empty set on line 52.

However, I do wonder if there is a possibility that an invalid handle could be returned by the C library in the following code fragment:

   (callback, search_result) = ffi.from_handle(handle)

Any thoughts on this would be appreciated.

@jodal jodal added 2.x bug labels Mar 14, 2014
@jodal jodal self-assigned this Mar 14, 2014
@jodal jodal added this to the v2.0.0a2 milestone Mar 14, 2014
@liamw9534

Ok, I'd like to close this issue. It is a bug in my code which causes the search object to be unreferenced in certain code paths causing the GC to remove the object which is subsequently referenced by the search.py callback routine.

@liamw9534 liamw9534 closed this Mar 14, 2014
@jodal
Member
jodal commented Mar 14, 2014

I was suspecting this could happen, as you can see from some of my remaining TODOs, ref. https://github.com/mopidy/pyspotify/blob/v2.x/develop/spotify/search.py#L57-L59. I'll leave this open until I've tried to address it and make this safe.

@jodal jodal reopened this Mar 14, 2014
@liamw9534

Yes, I saw that. Thanks for the reply. Basically, in this instance, my search timed out and the search object was no longer being referenced. I think it's an easy fix, by the way, just to add a try/except around the code in the callback routine, which tries to remove the callback handle from the set, and just ride the exception out.

@jodal jodal added a commit that closed this issue Mar 25, 2014
@jodal jodal Keep callback handles for album, artist, image, inbox, search, toplis…
…t alive

Even if the end user throws away all references to the object, the handle must
be kept alive until the complete/load callback for the object is called. If the
handle is GC-ed before the callback is called, the handle is potentially
garbage, and we may crash.

The solution chosen here is to add all handles to a package-global set,
spotify._callback_handles, and remove them again when the callback is called.

The only downside I can see with this solution is that if not all callbacks
is called we have a growing set of handle references in
spotify._callback_handles, keeping both handles and the associated objects and
callback functions alive. Hopefully this will be easy to detect/debug through
simple checks of len(spotify._callback_handles).

Fixes #121
f9834c0
@jodal jodal closed this in f9834c0 Mar 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment