fix: add fallback logic for CDN urls#1524
Conversation
|
This seems like a pretty clean solution, I like the direction.
It's been a while since I coded all of this, so I forgot how timeouts in this code path work. Do we need to do a
This is acceptable behavior to me. Yes, it'd be great to all of this exactly when the request happens. But like you say, it'd probably be a lot of work when the user could simply retry the request in the UI.
Also acceptable to me. Just saying that API breakage should not hold you up from doing something better.
Although I'd like to use something like a I vaguely remember that I created |
There was a problem hiding this comment.
Pull Request Overview
This PR adds fallback logic for CDN URLs to handle cases where the first CDN URL fails to connect. The solution involves returning all non-expired URLs from CdnUrl::try_get_urls() instead of just the first one, and implementing retry logic in AudioFileStreaming to try URLs sequentially until one works.
- Adds a new
try_get_urls()method to return all valid CDN URLs for fallback attempts - Modifies
AudioFileStreamingto iterate through URLs and retry on connection failures - Changes
stream_from_cdnAPI to accept a string URL instead ofCdnUrlobject
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/cdn_url.rs | Adds try_get_urls() method and deprecates try_get_url() for fallback support |
| core/src/spclient.rs | Changes stream_from_cdn to accept string URL parameter instead of CdnUrl |
| audio/src/fetch/mod.rs | Implements URL fallback logic in AudioFileStreaming and stores selected URL as string |
I'm not that familiar with the HTTP client part of librespot, but it seems like there is no timeout? Good catch, I'll take a look.
I guess using CdnUrl specifically made the API more foolproof by only allowing URLs returned from Spotify to be used, but that's not a big deal to be honest. Besides, if the user wants to use In terms of just using a general URL type, well... Rust doesn't have a native one. |
Cool, thanks.
This may in fact be necessary for external podcasts.
It will be converted into |
|
I'll look at the timeout as soon as possible, currently getting hit by #1527 which makes it difficult to test lol.
It is converted to |
|
Yes, I also think it's best to do that so we can fail early on an invalid URI. |
It seems that we do already though, session.spclient().stream_from_cdn("m:?c", 0, 1024)Fails with: Is this the early fail behavior you're talking about? After considering it for a while, I think there are only two sane options here. Either we keep pub fn stream_from_cdn<U>(&self, cdn_url: U, offset: usize, length: usize) -> Result<IntoStream<ResponseFuture>, Error>
where
U: TryInto<Uri>,
<U as TryInto<Uri>>::Error: Into<http::Error>The fail behavior is the same, but this allows the consumer to pass anything that the request builder would accept, not just |
|
In terms of timeouts, I tried firewalling off one of the the CDN hosts to see how Log: [2025-08-07T18:16:42Z WARN librespot_audio::fetch] Fetching https://audio-cf.spotifycdn.com/audio/f0a65b44aff025a6ea5a8f6d9dcbd43baff09865?verify=1754676734-Dli%2BvoShXhIsKLxw8wlyVEMQ3fZtc3P11WLqZXQRA20%3D
failed with error Error { kind: Unavailable, error: hyper_util::client::legacy::Error(Connect, Custom { kind: Other, error: ConnectError("tcp connect error", Os { code: 110, kind: TimedOut, message: "Connection timed out" }) }) }, trying next |
|
If you go for this, I think a much shorter timeout. If something takes 30 seconds I think people have already restarted it. Maybe 5? |
|
For now I went for 10 seconds, I personally feel like 5s is too short and with Spotify's long history of abysmal infrastructure performance I can easily imagine a download taking a bit longer and succeeding anyway. |
Yes, I like the second. Catch invalid input, and throw an error when we do, as fast as we can. |
|
Great, thanks a lot. Merged. |
|
Awesome, thanks both. |
|
Whoops, looks like I forgot to update the changelog after the last commit. It still says |
CdnUrlreturn all non-expired URLs instead of just the first one. Then, inAudioFileStreamingall URLs are tried and the first working URL is selected for later use.AudioFileFetch::download_range, but that is also a synchronous function so implementing that would probably require an overhaul of the codebase anyway, unless I'm missing something.stream_from_cdn, as it is not async (it doesn't actually send the request), so I made it accept the singular URL instead ofCdnUrl. It is the responsibility of the caller to implement URL fallback logic by inspecting the result of the streamer.stream_from_cdnanymore, but there are a few ways this could be fixed.SingleCdnUrl?CdnUrlthat would select the working URL and discard the rest?Example log
Closes: #1521