search

feelfreelinux / cspot

A Spotify Connect player targeting, but not limited to embedded devices (ESP32).
Other
495 stars 46 forks source link

fix deadlock in TrackPlayer during spirc::disconnect #158

Closed philippe44 closed 1 year ago

philippe44 commented 1 year ago

SpircHandler:: disconnect() does a TrackPlayer->resetState() but that's not enough because TrackPlayer will abort the current track and if there is a next one, it will grab it immediately and try to stream it.

It is expected that after disconnect(), SpircHandler instance will be deleted and so will it's TrackPlayer object. In its destructor, TrackPlayer sets isRunning to false but that will not exit the task if it is still trying to stream and we have a deadlock => SpircHandler can't be destroyed because TrackPlayer can't be.

The solution could be to just add a resetState() in TrackPlayer's destructor so that at least it exits the busy streaming loop and because isRunning is false it will fully exit as well, returning to SpircHandler destructor. Still, I think this is better to call TrackPlayer::stop() in SpircHandler::disconnect() so that the trackPlayer object is *actuallyù deactivated because it prevents it to try to grab another track. And it should not dot that because, AFAIU, there is no future for a disconnected spircHandler, except being destroyed.

By the same token, adding resetState() in TrackPlayer's destructor is then not mandatory if SpircHandler::disconnect calls TrackPlayer::stop() instead of resetState() because upon TrackPlayer's destruction, SpircHJandler has guaranteed it is not any more in the stream busy loop... well I'll let you judge of that, I prefer to have added that as a safety precaution in case I forgot something.

feelfreelinux commented 1 year ago

LGTM, I see no issue with keeping resetState in the destructor too, as it makes sure that the state will always be correct during destruction. I ran into this issue some time ago too, so this saves me quite some time :)