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

use async/await for mainloop #1059

Merged
merged 8 commits into from
Sep 6, 2022

Conversation

eladyn
Copy link
Member

@eladyn eladyn commented Feb 20, 2022

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
Copy link

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
Copy link
Member Author

eladyn commented Feb 20, 2022

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

This comment was marked as resolved.

@ckcr4lyf
Copy link

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
Copy link
Member Author

eladyn commented Feb 21, 2022

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
Copy link

Awesome, this MR gets a 👍 from me!!

@robinvd
Copy link
Contributor

robinvd commented Feb 24, 2022

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

@eladyn
Copy link
Member Author

eladyn commented Feb 26, 2022

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
Copy link
Member Author

eladyn commented Feb 27, 2022

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

src/main_loop.rs Outdated Show resolved Hide resolved
robinvd
robinvd previously approved these changes Mar 2, 2022
@robinvd robinvd requested a review from a team March 2, 2022 16:15
eladyn added a commit to eladyn/spotifyd that referenced this pull request Mar 4, 2022
@slondr
Copy link
Member

slondr commented Mar 11, 2022

Seems like we have merge conflicts here now.

Previously, handling too many librespot events in a row, the spawned hook might not have been finished. In this case, the events would stack up, since the event channel then would not be polled.

By using the tokio::process module, we gain support for waiting for processes asynchronously and even if an event would not be handled directly, it will be handled after the previous hook process has finished.
The mainloop code is simplified by relying on the async / await syntax
instead of implementing all the futures ourselves. In doing so, this
also works, if a different user connects during the playback.

Additionally, the application chooses now between connecting using the
users credentials and starting discovery. If there are valid
credentials, no discovery stream is started at all, which prevents
buggy and surprising behaviour.
eladyn and others added 2 commits March 12, 2022 00:13
Also remove an unneeded clone.
Co-authored-by: Robin van Dijk <robin@robinjint.nl>
@eladyn
Copy link
Member Author

eladyn commented Mar 11, 2022

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
Copy link

noctux commented Mar 17, 2022

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
Copy link

@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
Copy link
Member Author

eladyn commented Mar 17, 2022

@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
Copy link

noctux commented Mar 17, 2022

@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
Copy link

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
Copy link

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

robinvd
robinvd previously approved these changes Apr 26, 2022
@robinvd robinvd requested review from slondr and a team April 26, 2022 09:28
@noctux
Copy link

noctux commented Apr 29, 2022

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 eladyn mentioned this pull request May 6, 2022
5 tasks
This works around the issue, where dbus servers would stay around until
the application ends, although their corresponding session had already
ended. New sessions were then unable to communicate through dbus. Now,
newer servers will just replace any previous name owners.
@eladyn
Copy link
Member Author

eladyn commented May 7, 2022

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants