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

core: retry session connection #1345

Merged
merged 1 commit into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ https://github.com/librespot-org/librespot
- [core] `FileId` is moved out of `SpotifyId`. For now it will be re-exported.
- [core] Report actual platform data on login
- [core] Support `Session` authentication with a Spotify access token
- [core] `Credentials.username` is now an `Option` (breaking)
- [core] `Credentials.username` is now an `Option` (breaking)
- [core] `Session::connect` tries multiple access points, retrying each one.
- [main] `autoplay {on|off}` now acts as an override. If unspecified, `librespot`
now follows the setting in the Connect client that controls it. (breaking)
- [metadata] Most metadata is now retrieved with the `spclient` (breaking)
Expand Down
23 changes: 23 additions & 0 deletions core/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,29 @@ pub async fn connect(host: &str, port: u16, proxy: Option<&Url>) -> io::Result<T
handshake(socket).await
}

pub async fn connect_with_retry(
host: &str,
port: u16,
proxy: Option<&Url>,
max_retries: u8,
) -> io::Result<Transport> {
let mut num_retries = 0;
loop {
match connect(host, port, proxy).await {
Ok(f) => return Ok(f),
Err(e) => {
debug!("Connection failed: {e}");
if num_retries < max_retries {
num_retries += 1;
debug!("Retry access point...");
continue;
}
return Err(e);
}
}
}
}

pub async fn authenticate(
transport: &mut Transport,
credentials: Credentials,
Expand Down
29 changes: 23 additions & 6 deletions core/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,15 @@ impl Session {

async fn connect_inner(
&self,
access_point: SocketAddress,
access_point: &SocketAddress,
credentials: Credentials,
) -> Result<(Credentials, Transport), Error> {
let mut transport = connection::connect(
const MAX_RETRIES: u8 = 1;
let mut transport = connection::connect_with_retry(
&access_point.0,
access_point.1,
self.config().proxy.as_ref(),
MAX_RETRIES,
kingosticks marked this conversation as resolved.
Show resolved Hide resolved
)
.await?;
let mut reusable_credentials = connection::authenticate(
Expand All @@ -165,10 +167,11 @@ impl Session {
trace!(
"Reconnect using stored credentials as token authed sessions cannot use keymaster."
);
transport = connection::connect(
transport = connection::connect_with_retry(
&access_point.0,
access_point.1,
self.config().proxy.as_ref(),
MAX_RETRIES,
)
.await?;
reusable_credentials = connection::authenticate(
Expand All @@ -187,19 +190,32 @@ impl Session {
credentials: Credentials,
store_credentials: bool,
) -> Result<(), Error> {
// There currently happen to be 6 APs but anything will do to avoid an infinite loop.
const MAX_AP_TRIES: u8 = 6;
let mut num_ap_tries = 0;
let (reusable_credentials, transport) = loop {
let ap = self.apresolver().resolve("accesspoint").await?;
info!("Connecting to AP \"{}:{}\"", ap.0, ap.1);
match self.connect_inner(ap, credentials.clone()).await {
match self.connect_inner(&ap, credentials.clone()).await {
Ok(ct) => break ct,
Err(e) => {
num_ap_tries += 1;
if MAX_AP_TRIES == num_ap_tries {
error!("Tried too many access points");
return Err(e);
}
if let Some(AuthenticationError::LoginFailed(ErrorCode::TryAnotherAP)) =
e.error.downcast_ref::<AuthenticationError>()
{
warn!("Instructed to try another access point...");
continue;
} else {
} else if let Some(AuthenticationError::LoginFailed(..)) =
e.error.downcast_ref::<AuthenticationError>()
kingosticks marked this conversation as resolved.
Show resolved Hide resolved
{
return Err(e);
} else {
warn!("Try another access point...");
continue;
}
}
}
Expand Down Expand Up @@ -567,7 +583,7 @@ impl Session {
}

pub fn shutdown(&self) {
debug!("Invalidating session");
debug!("Shutdown: Invalidating session");
self.0.data.write().invalid = true;
self.mercury().shutdown();
self.channel().shutdown();
Expand Down Expand Up @@ -624,6 +640,7 @@ where
return Poll::Ready(Ok(()));
}
Some(Err(e)) => {
error!("Connection to server closed.");
session.shutdown();
return Poll::Ready(Err(e));
}
Expand Down