use async/await for mainloop

[eladyn]

Another PR by me! 😅

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,

[ckcr4lyf]

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?

[eladyn]

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!

[ckcr4lyf]

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 <simon@flaskpost.org>, Sven Lechner <sven.lechner@rwth-aachen.de>
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]

[eladyn]

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

[ckcr4lyf]

Awesome, this MR gets a 👍 from me!!

[robinvd]

looks very cool! dont have time right now, but ill review later

[eladyn]

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 (rust-lang/rust-clippy#8054)

[eladyn]

I rebased this on the current master and CI is fine now (thanks for merging #1062 that quickly!).

[slondr]

Seems like we have merge conflicts here now.

[eladyn]

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

[noctux]

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 3b7ee18 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...

[ckcr4lyf]

@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.

[eladyn]

@noctux @ckcr4lyf The missing reconnection logic is definitely a long standing problem (see e.g. #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 (librespot-org/librespot#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!

[noctux]

@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

[ckcr4lyf]

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

[ckcr4lyf]

Bump, would be nice to have this merged in (currently am using this fork as my main)

[noctux]

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)

[eladyn]

As outlined in #1079 (comment), 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!

[SimonTeixidor]

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.