Fix panic in ApResolver::resolve (#1038)

- Fixed resolve function panicking when resolving endpoint type with no
  AP in the list
- Fixed fallback APs not being applied when only some of the AP types
  were missing
- Switch container type from `Vec` to `VecDeque` for the `AccessPoints`
- Remove the note about fallback AP being used even if the port is not
  matching the configured `ap_port`

core/src/apresolve.rs

@@ -1,3 +1,5 @@
+use std::collections::VecDeque;
+
 use hyper::{Body, Method, Request};
 use serde::Deserialize;
 
@@ -7,23 +9,23 @@ pub type SocketAddress = (String, u16);
 
 #[derive(Default)]
 pub struct AccessPoints {
-    accesspoint: Vec<SocketAddress>,
-    dealer: Vec<SocketAddress>,
-    spclient: Vec<SocketAddress>,
+    accesspoint: VecDeque<SocketAddress>,
+    dealer: VecDeque<SocketAddress>,
+    spclient: VecDeque<SocketAddress>,
 }
 
-#[derive(Deserialize)]
+#[derive(Deserialize, Default)]
 pub struct ApResolveData {
     accesspoint: Vec<String>,
     dealer: Vec<String>,
     spclient: Vec<String>,
 }
 
-// These addresses probably do some geo-location based traffic management or at least DNS-based
-// load balancing. They are known to fail when the normal resolvers are up, so that's why they
-// should only be used as fallback.
-impl Default for ApResolveData {
-    fn default() -> Self {
+impl ApResolveData {
+    // These addresses probably do some geo-location based traffic management or at least DNS-based
+    // load balancing. They are known to fail when the normal resolvers are up, so that's why they
+    // should only be used as fallback.
+    fn fallback() -> Self {
         Self {
             accesspoint: vec![String::from("ap.spotify.com:443")],
             dealer: vec![String::from("dealer.spotify.com:443")],
@@ -32,6 +34,12 @@ impl Default for ApResolveData {
     }
 }
 
+impl AccessPoints {
+    fn is_any_empty(&self) -> bool {
+        self.accesspoint.is_empty() || self.dealer.is_empty() || self.spclient.is_empty()
+    }
+}
+
 component! {
     ApResolver : ApResolverInner {
         data: AccessPoints = AccessPoints::default(),
@@ -49,27 +57,34 @@ impl ApResolver {
         }
     }
 
-    fn process_data(&self, data: Vec<String>) -> Vec<SocketAddress> {
+    fn process_ap_strings(&self, data: Vec<String>) -> VecDeque<SocketAddress> {
+        let filter_port = self.port_config();
         data.into_iter()
             .filter_map(|ap| {
                 let mut split = ap.rsplitn(2, ':');
                 let port = split.next()?;
-                let host = split.next()?.to_owned();
                 let port: u16 = port.parse().ok()?;
-                if let Some(p) = self.port_config() {
-                    if p != port {
-                        return None;
+                let host = split.next()?.to_owned();
+                match filter_port {
+                    Some(filter_port) if filter_port != port => None,
+                    _ => Some((host, port)),
                 }
-                }
-                Some((host, port))
             })
             .collect()
     }
 
+    fn parse_resolve_to_access_points(&self, resolve: ApResolveData) -> AccessPoints {
+        AccessPoints {
+            accesspoint: self.process_ap_strings(resolve.accesspoint),
+            dealer: self.process_ap_strings(resolve.dealer),
+            spclient: self.process_ap_strings(resolve.spclient),
+        }
+    }
+
     pub async fn try_apresolve(&self) -> Result<ApResolveData, Error> {
         let req = Request::builder()
             .method(Method::GET)
-            .uri("http://apresolve.spotify.com/?type=accesspoint&type=dealer&type=spclient")
+            .uri("https://apresolve.spotify.com/?type=accesspoint&type=dealer&type=spclient")
             .body(Body::empty())?;
 
         let body = self.session().http_client().request_body(req).await?;
@@ -82,30 +97,33 @@ impl ApResolver {
         let result = self.try_apresolve().await;
 
         self.lock(|inner| {
-            let data = match result {
-                Ok(data) => data,
-                Err(e) => {
-                    warn!("Failed to resolve access points, using fallbacks: {}", e);
-                    ApResolveData::default()
-                }
+            let (data, error) = match result {
+                Ok(data) => (data, None),
+                Err(e) => (ApResolveData::default(), Some(e)),
             };
 
-            inner.data.accesspoint = self.process_data(data.accesspoint);
-            inner.data.dealer = self.process_data(data.dealer);
-            inner.data.spclient = self.process_data(data.spclient);
+            inner.data = self.parse_resolve_to_access_points(data);
+
+            if inner.data.is_any_empty() {
+                warn!("Failed to resolve all access points, using fallbacks");
+                if let Some(error) = error {
+                    warn!("Resolve access points error: {}", error);
+                }
+
+                let fallback = self.parse_resolve_to_access_points(ApResolveData::fallback());
+                inner.data.accesspoint.extend(fallback.accesspoint);
+                inner.data.dealer.extend(fallback.dealer);
+                inner.data.spclient.extend(fallback.spclient);
+            }
         })
     }
 
-    fn is_empty(&self) -> bool {
-        self.lock(|inner| {
-            inner.data.accesspoint.is_empty()
-                || inner.data.dealer.is_empty()
-                || inner.data.spclient.is_empty()
-        })
+    fn is_any_empty(&self) -> bool {
+        self.lock(|inner| inner.data.is_any_empty())
     }
 
     pub async fn resolve(&self, endpoint: &str) -> Result<SocketAddress, Error> {
-        if self.is_empty() {
+        if self.is_any_empty() {
             self.apresolve().await;
         }
 
@@ -114,9 +132,9 @@ impl ApResolver {
                 // take the first position instead of the last with `pop`, because Spotify returns
                 // access points with ports 4070, 443 and 80 in order of preference from highest
                 // to lowest.
-                "accesspoint" => inner.data.accesspoint.remove(0),
-                "dealer" => inner.data.dealer.remove(0),
-                "spclient" => inner.data.spclient.remove(0),
+                "accesspoint" => inner.data.accesspoint.pop_front(),
+                "dealer" => inner.data.dealer.pop_front(),
+                "spclient" => inner.data.spclient.pop_front(),
                 _ => {
                     return Err(Error::unimplemented(format!(
                         "No implementation to resolve access point {}",
@@ -125,6 +143,13 @@ impl ApResolver {
                 }
             };
 
+            let access_point = access_point.ok_or_else(|| {
+                Error::unavailable(format!(
+                    "No access point available for endpoint {}",
+                    endpoint
+                ))
+            })?;
+
             Ok(access_point)
         })
     }

src/main.rs

@@ -561,7 +561,7 @@ fn get_setup() -> Setup {
     .optopt(
         AP_PORT_SHORT,
         AP_PORT,
-        "Connect to an AP with a specified port 1 - 65535. If no AP with that port is present a fallback AP will be used. Available ports are usually 80, 443 and 4070.",
+        "Connect to an AP with a specified port 1 - 65535. Available ports are usually 80, 443 and 4070.",
         "PORT",
     );