spotifyd
spotifyd copied to clipboard
Spotifyd
use async/await for mainloop
Another PR by me! :sweat_smile:
This builds on top of #1047 (the first 4 commits are from that PR) and reimplements the mainloop using the async/await syntax instead of implementing the futures and the polling all manually. While doing that, I also added the small fix from #1041. This turns the quite complex impl Future for the MainLoopState into something more linear and readable (at least I hope so).
My original motivation for this PR was to implement a config flag that would allow the user to disable the zeroconf discovery (#1057). While I was doing that, I realized that it'd make much more sense to decide automatically, wether we should use discovery or not. Thus, this PR implements the following behaviour:
- if enough information is present (username and password, provided through whichever method) to authenticate, spotifyd connects to Spotify using that information and does not start a discovery stream
- otherwise, it starts the discovery and authenticates the session, when a user selects the Spotify Connect device
By doing so, we basically get #1057 for free, since we either run as a bare Spotify Connect device that is open to everyone or as a playback device that is bound to one user. Note: This does not resolve the current issues with credentials caching (#1019, #948).
Another issue that is (as far as I can tell) resolved with this PR is spotifyd exiting on subsequent connections from different users.
closes #1041 fixes #1057, fixes #1036,
Cheers mate, I do not know enough rust to be able to review, but going through doesn't seem like there's something to steal my credentials ;)
I guess I just do:
cargo build
yeah?
I guess I just do:
cargo build
Yeah, you should be fine just running this. If you need any of the features that are not enabled by default, here would be how to do that. And you can always turn on --release to enable some optimizations (while reducing the debuggability of the binary.)
Hope it works for you!
@eladyn this MR doesn't seem to fix the zeroconf issue, this is what I get:
On Launch:
$ ./spotifyd --no-daemon
Loading config from "/home/raghu/.config/spotifyd/spotifyd.conf"
No password specified. Checking password_cmd
Running "pass spotify" using "/bin/bash"
No proxy specified
Using software volume controller.
Connecting to AP "ap-gae2.spotify.com:443"
Authenticated as "[REDACTED]" !
Using alsa sink
Country: "HK"
Checking ports, it is listening on the zeroconf still
$ netstat -ntlpu
(Not all processes could be identified, non-owned process info
will not be shown, you would have to be root to see it all.)
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name
tcp 0 0 127.0.0.1:5037 0.0.0.0:* LISTEN 3387722/adb
tcp 0 0 0.0.0.0:1234 0.0.0.0:* LISTEN 1756016/./spotifyd
udp 0 0 0.0.0.0:5353 0.0.0.0:* 1756016/./spotifyd
udp6 0 0 :::5353
Additionally, it seems this MR, when I build it, does not support pulseaudio (which is what my config was set to use). Dunno if its related to my build, but the official release does support it.
nvm I'm dumb, I didnt realize I still have to manually check out your branch (quite embarassing)...
It works great for the zeroconf! Launch logs:
$ ./spotifyd --no-daemon
Loading config from "/home/raghu/.config/spotifyd/spotifyd.conf"
No password specified. Checking password_cmd
Running "pass spotify" using "/bin/bash"
No proxy specified
Using software volume controller.
Connecting to AP "ap.spotify.com:443"
Authenticated as "[REDCTED]" !
Using Alsa sink with format: S16
Country: "HK"
Netstat:
$ netstat -ntlpu
(Not all processes could be identified, non-owned process info
will not be shown, you would have to be root to see it all.)
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name
tcp 0 0 127.0.0.1:5037
However, I had to change my backend to alsa , I don't know if it's the branch or how I built it (naive cargo build), but seems it does not have support for pulseaudio.
Running --help :
$ ./spotifyd --help
spotifyd 0.3.3
Simon Persson <[email protected]>, Sven Lechner <[email protected]>
A Spotify daemon
USAGE:
spotifyd [FLAGS] [OPTIONS]
FLAGS:
--autoplay Autoplay on connect
--debug-credentials Whether the credentials should be debugged
-h, --help Prints help information
--no-audio-cache Disable the use of audio cache
--no-daemon If set, starts spotifyd without detaching
-V, --version Prints version information
--verbose Prints more verbose output
--volume-normalisation Enable to normalize the volume during playback
OPTIONS:
-b, --backend <string> The audio backend to use [possible values: alsa]
-B, --bitrate <number>
The bitrate of the streamed audio data [possible values: 96, 160, 320]
Great to hear that it works for you!
As for pulseaudio, you should be able to build via cargo build --features "pulseaudio_backend" (--release) (the last part is optional, but it usually leads to better performance).
This CI failure seems unrelated (probably a random networking issue). Although apparently the map_flatten clippy lint has been somehow extended and now complains about two parts (here and here), which I haven't touched.
Edit: apparently the map_flatten lint was just promoted to warn-by-default (https://github.com/rust-lang/rust-clippy/pull/8054)
I rebased this on the current master and CI is fine now (thanks for merging #1062 that quickly!).
I rebased this on top of current master and all merge conflicts should be resolved now. Note however that this still depends on changes in #1047 and that PR should probably go first (except if you just want to merge both at once, of course).
Hey @eladyn, thank you for the nice PR, I (as a pure user) was really hoping for a fix for #1057. Something that I noticed though is that (when running this PR, or more precisely 3b7ee18e37a81e0c6614280a6e205cac71b31f0c from your pr_collection branch) I often receive stale connections. I'm running spotifyd on a laptop, and usually after suspend I have to manually restart spotifyd because the playbackdevice offered via spotifyd is no longer visible. Verbose logging shows no activity after resuming from suspend. I guess that due to the hanging/now timeouted TCP-session, a reconnection has to be made. I'm not sure that this is actually a new problem, but may very well just now be as clearly visible (because before, due to the mDNS-advertisment there was a second communication vector that could make spotifyd initiate a new connection to spotify's servers). The issue should probably also be no blocker to this particular PR. I don't really know how to properly fix this past sending manual heartbeats to check that the connection is still valid...
@noctux now that you mention it, I have also faced this issue. After a while of idling, my phone will no longer show the daemon. I kinda shrugged it off and just restarted spotifyd, but I guess it may be the heartbeat thing you mentioned as well.
@noctux @ckcr4lyf The missing reconnection logic is definitely a long standing problem (see e.g. https://github.com/Spotifyd/spotifyd/issues/903). Unfortunately, I don't think that there is anything we can do about that on the spotifyd side since librespot currently does not really support handling connection errors properly (https://github.com/librespot-org/librespot/discussions/609).
I agree that using this PR with an authorized user (and thus not with the zeroconf auth) makes this error more problematic, since it is no longer possible to recover from it.
However, previously after a connection problem, you would have had to be on the same network to reconnect to the device. (Otherwise it would not be visible in the devices list.)
With this PR, if you are on the same network, there is not much benefit from using user authentication anyway and you can just use zeroconf auth like before. If you are on a different network, the situation is the same as before: You can't trigger a reconnect, since the device does not appear in the list.
So I would argue that this PR does not change the (admittedly annoying) situation much. Thanks for the feedback nonetheless!
@eladyn Thanks for the pointers and your work. As I said I don't think that this should in any way influence the acceptance of this PR :D And from an privacy/security point of view this PR is a definitive win. I just wanted to document it here, in case other people search for it or you weren't aware of it. No worries :D And I can probably always restart it manually is music does not start playing
Actually today after idling for couple of hours, was still able to connect to it. But anyway yeah that might be a deeper issue, the security benefit is much more important to me
Bump, would be nice to have this merged in (currently am using this fork as my main)
Adding to the reconnect issue:
Actually, at times this code does detect that my laptop was in suspend:
Loading config from "<censored>/spotifyd.conf"
No password specified. Checking password_cmd
No password_cmd specified
No proxy specified
Using software volume controller.
Checking keyring for password
Connecting to AP "ap.spotify.com:443"
Authenticated as "<censored>" !
Country: "<censored>"
Using PulseAudio sink with format: S16
<... plays some songs>
subscription terminated
Connection reset by peer (os error 104)
Connecting to AP "ap.spotify.com:443"
Authenticated as "<censored>" !
Country: "<censored>"
Using PulseAudio sink with format: S16
After that, the device is not available as a sink, neither in the webplayer nor in other frontends such as spotify-qt or spotify-tui. I'm not sure if "relogin" needs a device-reregistering step there. (Edit: and no further output by spotifyd happens after that point)
As outlined in https://github.com/Spotifyd/spotifyd/pull/1079#issuecomment-1120065814, I noticed that the new possibility of re-using spotifyd multiple times would break the dbus server after the first time, since it wasn't properly cleaned up. My current workaround is quite dirty (thus the TODO), but as far as I can tell, releasing the name (and closing the connection) is not that easy, and I didn't want to clutter this PR even more.
If you have different suggestions, please let me know. I hope that this was the last change that needs to be made and that this PR is finally ready to be merged. Thank you for your time!
Thanks a lot for this work! It looks good to me, and I have merged. I will go through some other open PRs and see if I can put together a (long overdue) release.
