Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Random deadlocks during notify_main_thread() callback #86

Closed
jkp opened this Issue · 6 comments

4 participants

@jkp
jkp commented

I didn't really know what to name this issue because the problem is actually potentially in other places but this is the one place I know it exists. Take a look at the following backtraces:

 Thread 13

 (gdb) bt 10
 #0  0x9302191a in __psynch_mutexwait ()
 #1  0x98e4c13b in pthread_mutex_lock ()
 #2  0x012bc769 in g_session ()
 #3  0x012bc8bd in g_session ()
 #4  0x0132a9f7 in g_session ()
 #5  0x01331dfa in g_session ()
 #6  0x012f658f in g_session ()
 #7  0x013a6ff0 in sp_albumbrowse_create ()
 #8  0x0125d802 in AlbumBrowser_new ()
 #9  0x0008f76a in type_call (type=0x1264a98, args=0x14a6328, kwds=0x0) at Objects/typeobject.c:422

 Thread 12

 (gdb) bt
 #0  0x930218e2 in __psynch_cvwait ()
 #1  0x98e4b220 in _pthread_cond_wait ()
 #2  0x98ed10a1 in pthread_cond_wait$UNIX2003 ()
 #3  0x0012ef20 in PyThread_acquire_lock (lock=0x7a97f0e0, waitflag=1) at thread_pthread.h:452
 #4  0x000df971 in PyEval_RestoreThread (tstate=0x7c267f30) at Python/ceval.c:364
 #5  0x0011901d in PyGILState_Ensure () at Python/pystate.c:592
 #6  0x0125bb22 in notify_main_thread ()
 #7  0x012bc5eb in g_session ()
 #8  0x012dd6f7 in g_session ()
 #9  0x012d0ceb in g_session ()
 #10 0x012d47a1 in g_session ()
 #11 0x012d2dd9 in g_session ()
 #12 0x012cccf3 in g_session ()
 #13 0x012ce6c9 in g_session ()
 #14 0x0137be72 in g_session ()
 #15 0x0137be10 in g_session ()
 #16 0x0137daa9 in g_session ()
 #17 0x0137d8bf in g_session ()
 #18 0x012a1228 in g_session ()
 #19 0x98e46557 in _pthread_start ()
 #20 0x98e30cee in thread_start ()

The problem is that the Spofify notify callback is being invoked whilst the GIL is held by another thread: that thread in turn is blocking on something in g_session, which I can only assume is being held by the notify thread.

Now you could easily argue that libspotify should guarantee that this will never happen, but alas, that doesn't seem to be the case.

One possible fix for this would be to release the GIL before making a Spotify API call and re-aquire it when the call finishes. It's not the prettiest of things but it's probably the only safe thing to do.

For what it's worth I have spent hours and hours trying to track this down - it happens extremely regularly in my application (I'm the maintainer of the Plex Spotify plugin) - I'm pretty sure this isn't the only case where this is possible.

If you agree that this is a good solution to the issue I'm happy to run up a pull-request.

@adamcik
Owner

Wrapping the right code in the allow threads macro might do the trick. But I suspect that would also surface the other underlying issue which is that it would be fairly simple to use pyspotify in a way which isn't thread safe, as libspotify isn't.

https://developer.spotify.com/technologies/libspotify/faq/ has some notes on spotify and thread safety worth looking at.

@jodal
Owner

I know pyspotify 1.x doesn't release the GIL for some "getter" libspotify function calls that should be cheap and fast. Though, in comparision, pyspotify 2.x releases the GIL for all libspotify function calls, since it use cffi which doesn't let you handle that from case to case. One could argue that releasing the GIL more often may reduce performance, but if that helps this issue, I'm +1 to wrapping more calls with the GIL release macro.

@kingosticks

The FAQ Adam references says:

session.notify_main_thread
session.end_of_track
session.music_delivery
session.start_playback
session.stop_playback
session.get_audio_buffer_stats
You should not call any other libspotify API methods from within these callbacks

But we call sp_session_userdata in all of these... Is that ok?

notify_main_thread in particular can be called from any one of libspotify's internal threads as well as:

It’s important to remember that the main thread can also invoke this callback

Do we need to be more careful with this callback?

@adamcik
Owner

s/Adam/Thomas/ - adamcik is just my last name :-)

As for sp_session_userdata,all libspotify has is the void pointer we gave them, which it can't modify as it has no idea what it is storing. Since it's tied to a given session it can't be modified behind our backs because the global session gets replaced, so in other words I think there is no danger in this.

But in general it feels a bit fishy to be storing pointer to python objects deep inside spotify, especially when you consider that we have a single session as global state...

So my point is really that I'm not 100% sure about the ramifications, which might already be mitigated by the virtue of being stuck with the GIL and which ones might need fixing.

@kingosticks

Oh sorry Thomas I wasn't thinking straight that late!

@jodal
Owner

Due to the imminent release of the 2.0 rewrite of pyspotify, this will probably never be done. Closing.

@jodal jodal closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.