Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix deadlock in TrackPlayer during spirc::disconnect #158

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

philippe44
Copy link
Contributor

@philippe44 philippe44 commented Oct 1, 2023

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.

@philippe44 philippe44 force-pushed the master branch 2 times, most recently from cc05245 to d240997 Compare October 1, 2023 23:28
@feelfreelinux
Copy link
Owner

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 :)

@feelfreelinux feelfreelinux merged commit d2b3ae3 into feelfreelinux:master Oct 4, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants