mirror of
https://github.com/librespot-org/librespot.git
synced 2026-04-27 08:15:50 +03:00
[GH-ISSUE #134] Handle reconnection for Sessions #103
Labels
No labels
A-Alsa
SpotifyAPI
Tokio 1.0
audio
bug
can't reproduce
compilation
dependencies
duplicate
enhancement
good first issue
help wanted
high priority
imported
imported
invalid
new api
pull-request
question
reverse engineering
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/librespot#103
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @sashahilton00 on GitHub (Feb 8, 2018).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/134
This issue serves as a placeholder for discussion around the rewrite of the session handling logic, as it is currently one of the less stable parts of librespot. Issue #103 is related to this.
@plietar commented on GitHub (Feb 10, 2018):
So here's my brain dump about the whole reconnection thing.
Currently one
Sessionobject is associated with one TCP connection to Spotify servers. We can choose to keep it this way and start returning appropriate errors to the caller, making itmain.rs(or any user of librespot) responsible for creating a new Session and switching to that. This is pretty tricky and puts the burden on every end application using librespot. Note however that the "switch session" part already kind of exists when switching users with discovery.The alternative is to move to a model where a
Sessionobject can alternate back and forth between a connected and a not connected state. The API forSessionbecomes something like :The
connectfunction can be called multiple times (switching users when discovery is enabled for example), and should be called again when a disconnection is noticed.Trying to use an unconnected session (to get metadata, audio key/data, etc...) should fail gracefully. All the APIs already use Future/Stream, so these just need to resolve into errors. Same happens if the connection is lost while a request is in flight.
Player and Spirc need to behave correctly when those requests fail. For Player it just means returning to the NotPlaying state and signal the caller that playback has completed with an error. For Spirc, when Player signals a connection problem it should probably stop playback. Reading from or sending to the mercury channel may start failing as well.
Now to get back up, I suggest the Session signals in some way to
mainthat connection was closed, andmainis responsible for attempting to reconnect by callingconnectagain, potentially with some exponential back off. It should do so by using theCredentialthat was returned by the last successfulconnectcall. The various components get notified of this and need to carry over with the new connection.This can mean various things depending on the component.
Spircfor example should reset its state if the username is different, and reestablish the mercury channel.The components need to expose an API that looks like:
There are some risks of race conditions that need to be carefully though about.
I think I have some initial implementation of this somewhere, I'll try and find it again.
@sashahilton00 commented on GitHub (Feb 17, 2018):
@plietar did you find that initial implementation anywhere?
@sashahilton00 commented on GitHub (Feb 26, 2018):
Regardless, I think we should be aiming to make this as simple to integrate as possible, as handling reconnection logic from whatever project uses librespot sounds like plenty of headache for anyone who tries to use librespot, whilst also being pretty low level functionality that I personally would expect to be handled in any library I used. Anyway, these are just my two cents since I'm not sufficently versed in rust to be able to rewrite how sessions are handled. Would be good to hear others thoughts as to how this should be handled.
@lrbalt commented on GitHub (Apr 30, 2018):
I am trying to wrap my head around this issue.
AFAICS, the disconnect error starts at
Session::DispatchTask::poll. The error causes theinvalidflag to be set onSession(patch #206) and the error is returned from the future and picked up by themap_erronSession::connectwhich only logs an error. BothSpirc::pollandPlayer::pollcheck on a valid session and they shut down if they notice that the session is invalid, freeing all resources likemercurySince
Playeris managed bySpirc, I think we can improve this by only checking onSessionatSpirc::polland letspircinformPlayerto stop/shutdown.maincurrently does not notice the lost connection at all which is probably wrong since it does not clean upspircandspirc_taskandplayer_event_channel. But they are replaced when a new connection is made, so no real harm done. This can be improved by lettingspirc_taskreturn an error and handle this inmain.So this works for the binary case.
For the library case @plietar suggests to let main reconnect using last credentials and let Spirc, Player, etc. pick up the new session and reset its state accordingly. I have two problems trying to implement this
'staticlifetime. Making Session::new causes the Session state to be non-static. I couldn't getSession::new()followed bySession::connect(&mut self)to work because &self not being'staticSession/Spirc/Playerfrom the credentials that main manages anyway? We could letSpircreturn with aConnectionError? I'm trying to figure out the use case.If you look at examples/play.rs you can do a match on
core.run(player.load(track, true, 0))and handle theConnectionError??@maciex commented on GitHub (Nov 21, 2018):
Is there any way to mitigate this problem?
Is it possible to somehow detect that it disconnected and restart it manually (using some shell script)?
@oboote commented on GitHub (Nov 26, 2018):
This doesn't address the cause but aids with the symptoms:
https://gist.github.com/JeremieRapin/bc35a2632c072a082d48f5503270df16
Script to be run as a cronjob every minute which checks the status and restarts the service if an error is spotted.
@cdlenfert commented on GitHub (Jul 29, 2019):
Is the cronjob patch the best solution for this issue currently? Sometimes I'm 25 songs in, sometimes I'm 4 when I lose connection. This is running on a server connected directly to my modem/router via ethernet. I can't imagine it's actually losing internet connection this consistently.
@willstott101 commented on GitHub (Oct 19, 2019):
Sounds like this is closely related to, and would probably help solve #266