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

do not panic on connection reset #206

Merged
merged 2 commits into from
Apr 25, 2018
Merged

Conversation

lrbalt
Copy link
Contributor

@lrbalt lrbalt commented Apr 21, 2018

If Spotify cannot communicate with your Librespot device, the connection is reset. This patch handles this case by stopping the Session, Spirc and Player. Librespot then keeps listening for new connections

This should fix #196 and #134

@fherrera124
Copy link

fherrera124 commented Apr 23, 2018

Tested on rp3 with Raspbian. Now it handles better when connection is unstable, but sometimes it crashes anyways.

@lrbalt
Copy link
Contributor Author

lrbalt commented Apr 23, 2018

I’d like to implement a reconnect using the credentials of the session which connection was reset. I’m seeing a lot of resets. But with this patch at least Librespot does not panic and keeps running

@lrbalt
Copy link
Contributor Author

lrbalt commented Apr 23, 2018

If it crashes, do you have a stacktrace to see if that crash is related to this patch?

@sashahilton00
Copy link
Member

@lrbalt thanks for taking a stab at this long running bug :) I'll not merge it for now to give @elganzua124 a chance to provide a stack trace to see if it has anything to do with this PR and/or if there is anything that can be added if necessary. With regards to reconnecting with existing credentials, I think you could potentially use the auth blob that's stored/returned to reauthenticate?

@plietar
Copy link
Contributor

plietar commented Apr 23, 2018

Please do not close #134 when merging this.
While this is a good workaround (thanks @lrbalt), it only works for librespot as a binary, not librespot as a library. The more general solution in #134 should be implemented instead.

@fherrera124
Copy link

fherrera124 commented Apr 24, 2018

I can not reproduce the issue right now. I'll do some more tests tomorrow. For now it resolves Internet interruptions just fine:

 /usr/bin/librespot -n test -b 320 --initial-volume -u username -p pass
INFO:librespot: librespot  (). Built on 2018-04-23. Build ID: 9x8FJj6v
INFO:librespot_core::session: Connecting to AP "guc3-accesspoint-b-fzk9.ap.spotify.com:4070"
INFO:librespot_core::session: Connecting to AP "guc3-accesspoint-b-lb9j.ap.spotify.com:4070"
INFO:librespot_core::session: Authenticated as "username" !
INFO:librespot_playback::audio_backend::alsa: Using alsa sink
INFO:librespot_core::session: Country: "AR"
INFO:librespot_playback::player: Loading track "Some track" with Spotify URI "spotify:track:blabla"
INFO:librespot_playback::player: Track "Some lyric" loaded
ERROR:librespot_core::session: Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" }
INFO:librespot_core::session: Connecting to AP "guc3-accesspoint-b-lqm9.ap.spotify.com:4070"
INFO:librespot_core::session: Authenticated as "username" !
INFO:librespot_playback::audio_backend::alsa: Using alsa sink
INFO:librespot_core::session: Country: "AR"
INFO:librespot_playback::player: Loading track "Some track" with Spotify URI "spotify:track:blabla"
INFO:librespot_playback::player: Track "Some track" loaded

Regarding to credentials, it would be great if librespot could reconnect using them, then one could choose in configurations if librespot should resume when session is restored (default) or just wait for user directives. With that I would be more than satisfied.

@lrbalt
Copy link
Contributor Author

lrbalt commented Apr 24, 2018

I can try to do the reconnect for #134. @plietar, are you available for review?

This PR can be merged in the mean time so that we do not panic on connection reset.

@sashahilton00
Copy link
Member

sashahilton00 commented Apr 24, 2018

Will merge this later this evening if no objections, and leave #134 open. Before I merge, @lrbalt could you run cargo fmt --all with the latest nightly and push a commit so that travis succeeds?

@lrbalt
Copy link
Contributor Author

lrbalt commented Apr 24, 2018

Sorry, I missed you cargo fmt question. I’ll do that tomorrow.

@lrbalt
Copy link
Contributor Author

lrbalt commented Apr 25, 2018

@sashahilton00 rustfmt added to PR

➜  librespot git:(master) ✗ rustc -V; cargo fmt --version
rustc 1.27.0-nightly (ac3c2288f 2018-04-18)
rustfmt 0.4.2-nightly (dd807e2 2018-04-18)

cargo fmt does complain about configuration options:

➜  librespot git:(master) ✗ cargo fmt --all
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `reorder_imports_in_group`

@sashahilton00
Copy link
Member

@lrbalt thanks, will let Travis run and merge.

@sashahilton00 sashahilton00 merged commit ffb7714 into librespot-org:master Apr 25, 2018
@sashahilton00
Copy link
Member

Merged, thanks for implementing this. Have left #134 open for when the reconnection logic is implemented in the library.

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.

crash after playback ends
4 participants