add session timeout handling

[eladyn]

See #1102 (comment).

This makes sure that we receive a ping at least every 2 minutes (which seems to happen consistently, when connected) and otherwise shuts down the session, since the connection stalled in this case. (inspired by the librespot-java implementation)

In combination with the changes to main.rs, this makes sure that librespot successfully reconnects.

I tested this by suspending my laptop and waking it up several hours later. After less than 2 minutes, it noticed that the connection had stalled and restarted itself.

(Hopefully) Fixes #1102.

[roderickvd]

Thanks for this. As mentioned on Gitter I'll be happy to remain here for assistance and code reviews, but no longer have a Premium account myself so that's that.

Overall question of design: there's a common Tokio pattern of having a sleep watchdog in the select! macro. Would it be softer on the eyes you think to have that watchdog in main they're calling some async session function, instead of a separate thread / task in Session itself?

As a downside, that would require downstream applications to implement such a watchdog themselves...

[eladyn]

As mentioned on Gitter I'll be happy to remain here for assistance and code reviews, but no longer have a Premium account myself so that's that.

Thank you for doing this and keeping the project alive!

Overall question of design: there's a common Tokio pattern of having a sleep watchdog in the select! macro. Would it be softer on the eyes you think to have that watchdog in main they're calling some async session function, instead of a separate thread / task in Session itself?

When implementing this, I wasn't sure either where to put this. Having something like async fn session_timeout(&self) on Session sounds like a good idea, that should also simplify the implementation. As for the responsibility of calling this function, would it make sense to put that into the SpircTask, which also has a select!? This way, we would keep the current propagation chain of things like broken channels (connection reset), which result in the spirc_task to finish. And we wouldn't have to put calling the timeout function into main, since the code there mainly hands the handling of the session object to the Spirc logic.

[roderickvd]

Thanks that latest change does make things much simpler! Any remaining points / questions apart from that single review comment I left? Looks good to me otherwise.

[roderickvd]

Did you get around to fixing the CI / dependency issue? Is it necessary to up MSRV?

[eladyn]

Oh, when I last pushed to this PR I saw the first few checks succeeding and assumed everything was fine.

Apparently, the cpal update did also break the MSRV, since the new version depends on windows 0.44. This does introduce the requirement of 1.64.0 as MSRV.¹

So I'm not entirely sure what to do: Should I bump the MSRV to 1.64.0? If so, would it be fine in here or rather in another PR?

Footnotes

1. The newest version of the windows crate actually decreases the MSRV again to 1.48.0. I opened a PR in cpal (https://github.com/RustAudio/cpal/pull/765) to update to that version. ↩

[roderickvd]

I see, thanks. Unless someone disagrees, I feel like merging this PR as-is then and accepting a broken Windows CI for the time being.

[kingosticks]

Sorry for being late to the party. Having this tied to the SpircTask is not ideal for people using librespot without Spirc, not everyone needs Spotify Connect. The concrete example case being GStreamer's spotifyaudiosrc but there could easily be others.

[roderickvd]

That's a good point. Pity of the current implementation, because it is much easier than the earlier one with all the threading going one. Any thoughts for an intermediate solution -- one that's not within SpircTask yet does not require magic threading powers?

[eladyn]

Hmm. With the current solution, the project you mentioned could implement that timeout handling themselves by simply keeping the session object and calling session.session_timeout().await in their “main loop” or whatever they're using.

The general problem here is:

• Ideally, I would've implemented it entirely within the session and communicated to the user how the session has failed. (At least I think that would be a good solution.) Then the user could decide to reconnect (or maybe an auto-reconnect option can be set). But that would - as far as I can see - require major refactoring.
• What I previously did was to do something similar without much refactoring: Shutting down and thus invalidating the session on a timeout. I could try to do that again in a cleaner way, without all the task scheduling / cancelling code.
• Another option would be to call the session_timeout function from the Player object, which probably most clients are using to do their stuff. I haven't looked into this option yet.

What do you think?

[roderickvd]

Indeed #1 will require major refactoring. It's necessary anyway if we ever want to get the dealer API in, as well as finally fix the reconnection logic once and for all, but I too consider it a major hurdle. I certainly don't expect you to do that as part of this scope.

I'm not a fan of #3 because most downstream packages actually have implemented their own player logic.

So that leaves #2 or "#0" with the implementation instruction to call session.session_timeout().await. While the latter does not seem like a big deal to me, it's also kind of crufty maybe.

Happy to hear any other thoughts.

[kingosticks]

Indeed, not everyone uses Player either.

So that leaves #2 or "#0"

I agree. I originally didn't like the application having to call session_timeout() but actually given where we are it is a nice way forward. Applications can then easy handle reconnection, if they desire.

On the subject of option 2, I had a go at a variant but I was wrestling with the Error types and couldn't even get it to compile so I gave up - sorry! Anyway, if it is interesting, it's at df29bee. Using weak() must be preferable to clone() but I just wanted to make it work (and failed at that).

[eladyn]

Assuming this is the route we want to take for now, I added some docs to Session (although basically nothing else is really documented - but you have to started somewhere) recommending using the session_timeout function for users that don't use Spirc. (And I rebased, which should fix the windows CI failure.)

[eladyn]

Anyway, if it is interesting, it's at df29bee. Using weak() must be preferable to clone() but I just wanted to make it work (and failed at that).

Oh, thank you for sharing. This also sounds like an interesting idea, especially just calling an async function instead of writing an entire struct for that. That might work as well. If you're generally in favor of this, I can certainly try making that work.

[roderickvd]

@eladyn would you be willing to revise the code for those ergonomics, so we can merge?

[eladyn]

I'll see if I can find the time this weekend. I assume you mean the proposal of @kingosticks in the direction of adding additional error variants?

[eladyn]

Sorry for taking so long. I finally found the time to try @kingosticks' proposal for handling the session timeout and just pushed the changes. It certainly feels like it might be easier to add the reconnection logic later on in this version.

Happy to take your feedback.

(Note: I removed the error variant from the session error struct that you added, since the try_join wants to deal with io::Errors anyway and the timeout is never visible outside the session.)

[kingosticks]

Thanks for coming back to it @eladyn. I think it's neat and gets my vote.

[roderickvd]

Indeed, thank you, looks very neat. If you would also add a changelog entry, then we can merge.

[eladyn]

Oh, now clippy found something new to complain about. Should I fix this here?

[roderickvd]

Yes if you would? So we can keep CI nice and clean. Thanks.

[kingosticks]

That is awesome, thanks both.

[hrkfdn]

Hey, thanks a lot for this improvement. Is there a release planned anytime soon? I believe downstream applications (e.g. ncspot) would really benefit from this (plus all the other goodies in dev).

[duczz]

any release date?

[kingosticks]

This project doesn't have planned release dates, so no. The other work required to release the main branch has not been completed and I don't think anyone is actively working on it. This is an issue that needs addressing.