SubmarinerApp / Submariner

A Subsonic client for macOS
https://submarinerapp.com
BSD 3-Clause "New" or "Revised" License
122 stars 3 forks source link

Connecting to a navidrome server with Album art rate limiting makes the application completely unusable #174

Closed lukstei closed 7 months ago

lukstei commented 7 months ago

Describe the bug When I connect to my Navidrome instance, which has a lot of songs, after a few seconds the rate limiting kicks in and causes errors in the requests, which shows an alert for each request and makes the application completely unusable.

Navidrome introduced rate limiting because of performance and rate limiting issues on external APIs, when too much album images are requested, see https://github.com/navidrome/navidrome/issues/2130 and related settings https://github.com/navidrome/navidrome/blob/bcab3cc0f908a01ebb99f9f6d0bfb37fadbd95bf/conf/configuration.go#L86-L88. The rate limiting is enabled by default.

To Reproduce Steps to reproduce the behavior:

  1. Connect to a navidrome instance with a lot of songs and rate limiting enabled
  2. Wait a few seconds
  3. See error

Expected behavior Application should still be usable, and respect the rate limiting (if possible). Maybe the 429s can just be ignored for now.

Screenshots https://github.com/SubmarinerApp/Submariner/assets/292363/403b0998-104f-4e2d-9167-16cec8a631ce

Versions:

Additional context Add any other context about the problem here.

NattyNarwhal commented 7 months ago

I have a preliminary patch for this:

diff --git a/Submariner/SBClientController.swift b/Submariner/SBClientController.swift
index 596b80c..20f3616 100644
--- a/Submariner/SBClientController.swift
+++ b/Submariner/SBClientController.swift
@@ -49,6 +49,29 @@ fileprivate let logger = Logger(subsystem: Bundle.main.bundleIdentifier!, catego
                 if response.statusCode == 404 {
                     self.server.markNotSupported(feature: type)
                     return
+                } else if response.statusCode == 429 {
+                    // Newer versions of Navidrome back getCoverArt w/ third-party APIs.
+                    // As such, it rate limits API requests that can invoke them.
+                    // Instead of bothering the user, retry the request later.
+
+                    // Retry-After is seconds or a specific date
+                    let retryAfter = response.value(forHTTPHeaderField: "Retry-After")
+                    logger.info("Retrying w/ Retry-After value \(retryAfter ?? "<nil>")")
+
+                    if let retryAfter = retryAfter,
+                       let specificDate = retryAfter.dateTimeFromHTTP() {
+                        _ = Timer(fire: specificDate, interval: 0, repeats: false) { timer in
+                            self.request(url: url, type: type, customization: customization)
+                            timer.invalidate()
+                        }
+                    } else if let retryAfter = retryAfter {
+                        let seconds = TimeInterval(retryAfter) ?? 5
+                        _ = Timer(timeInterval: seconds, repeats: false) { timer in
+                            self.request(url: url, type: type, customization: customization)
+                            timer.invalidate()
+                        }
+                    }
+                    return
                 } else if response.statusCode != 200 {
                     let message = "HTTP \(response.statusCode) for \(url.path)"
                     let userInfo = [NSLocalizedDescriptionKey: message]
diff --git a/Submariner/String+Time.swift b/Submariner/String+Time.swift
index 3544ea1..9653767 100644
--- a/Submariner/String+Time.swift
+++ b/Submariner/String+Time.swift
@@ -18,6 +18,13 @@ extension String {
         formatter.locale = Locale(identifier: "en_US_POSIX")
         return formatter
     }()
+    fileprivate static let httpDateFormatter: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.dateFormat = "EEE', 'dd' 'MMM' 'yyyy' 'hh:mm:ss' GMT'"
+        formatter.timeZone = TimeZone(secondsFromGMT: 0)
+        formatter.locale = Locale(identifier: "en_US_POSIX")
+        return formatter
+    }()

     static let componentFormatter: DateComponentsFormatter = {
         let formatter = DateComponentsFormatter()
@@ -34,6 +41,10 @@ extension String {
         return String.rfc3339DateFormatter.date(from: self as String)
     }

+    func dateTimeFromHTTP() -> Date? {
+        return String.httpDateFormatter.date(from: self)
+    }
+
     init(timeInterval: TimeInterval) {
         if timeInterval == 0 || timeInterval.isNaN {
             self = "00:00"

If you want, I can provide a binary build of this against the current development snapshot for you to test. I haven't tested it yet, I'm still on older Navidrome here.

lukstei commented 7 months ago

Hey @NattyNarwhal,

Thanks for the ultra fast reply!

Yes the patch works, I tested it and the 429 dialog is not appearing anymore.

However I believe the core problem is that I have a playlist with 3000 tracks. It seems like the app is trying to download the metadata for all the songs in the playlist at once. Then the server gets overwhelmed with requests, some are rejected with the 429, but the other still cause the server to be overloaded. A bunch of requests then are hitting the timeout and for those I'm seeing a "Request timed out" dialog. Because of these many requests to the server, it is not possible to start playing a song.

e.g.:

    API endpoint /rest/getCoverArt.view
Task <4533726C-C078-4698-9D01-5BF93C0BD2A9>.<1> finished with error [18.446.744.073.709.550.615] Error Domain=NSURLErrorDomain Code=-1001 "The request timed out." UserInfo={_kCFStreamErrorCodeKey=-2102, NSUnderlyingError=0x60000d0967f0 {Error Domain=kCFErrorDomainCFNetwork Code=-1001 "(null)" UserInfo={_kCFStreamErrorCodeKey=-2102, _kCFStreamErrorDomainKey=4}}, _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <4533726C-C078-4698-9D01-5BF93C0BD2A9>.<1>, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <4533726C-C078-4698-9D01-5BF93C0BD2A9>.<1>"
), NSLocalizedDescription=The request timed out., NSErrorFailingURLStringKey=http://raspberrypi:4533/rest/getCoverArt.view?t=XX&s=XX&id=mf-XX&v=1.15.0&c=submariner&u=lukas, NSErrorFailingURLKey=http://raspberrypi:4533/rest/getCoverArt.view?t=XX&s=XX&id=mf-XX&v=1.15.0&c=submariner&u=lukas, _kCFStreamErrorDomainKey=4}
Handling URL http://raspberrypi:4533/rest/getCoverArt.view?t=XX&s=XX&id=mf-XX&v=1.15.0&c=submariner&u=lukas
    API endpoint /rest/getCoverArt.view

Maybe the solution is to only download the metadata for the songs which are currently visible in the list?

lukstei commented 7 months ago

This quickfix makes the problem disappear (i.e. avoiding to fetch album covers for every song in the playlist):

diff --git a/Submariner/SBSubsonicParsingOperation.swift b/Submariner/SBSubsonicParsingOperation.swift
index aca281a..6d78522 100644
--- a/Submariner/SBSubsonicParsingOperation.swift
+++ b/Submariner/SBSubsonicParsingOperation.swift
@@ -397,7 +397,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 } ?? false
                 logger.info("Adding track (and updating) with ID: \(id, privacy: .public) to playlist \(currentPlaylist.itemId ?? "(no ID?)", privacy: .public), exists? \(exists) index? \(self.playlistIndex)")

-                updateTrackDependenciesForDirectoryIndex(track, attributeDict: attributeDict)
+                updateTrackDependenciesForDirectoryIndex(track, attributeDict: attributeDict, shouldFetchAlbumArt: false)

                 // limitation if the same track exists twice
                 track.playlistIndex = NSNumber(value: playlistIndex)
@@ -412,7 +412,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 // FIXME: Should we update *existing* tracks regardless? For previous cases they were pulled anew...
                 logger.info("Creating new track with ID: \(id, privacy: .public) for playlist \(currentPlaylist.itemId ?? "(no ID?)", privacy: .public)")
                 let track = createTrack(attributes: attributeDict)
-                updateTrackDependenciesForDirectoryIndex(track, attributeDict: attributeDict)
+                updateTrackDependenciesForDirectoryIndex(track, attributeDict: attributeDict, shouldFetchAlbumArt: false)

                 track.playlistIndex = NSNumber(value: playlistIndex)
                 playlistIndex += 1
@@ -884,7 +884,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
     }

     // not as good as former, but we have to use it until we switch to using tag based metadata instead of hierarchy index
-    private func updateTrackDependenciesForDirectoryIndex(_ track: SBTrack, attributeDict: [String: String]) {
+    private func updateTrackDependenciesForDirectoryIndex(_ track: SBTrack, attributeDict: [String: String], shouldFetchAlbumArt: Bool = true) {
         var attachedAlbum: SBAlbum?
         var attachedCover: SBCover?
         var attachedArtist: SBArtist?
@@ -925,7 +925,9 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 attachedAlbum.cover = attachedCover!
             }

-            clientController.getCover(id: coverID)
+            if shouldFetchAlbumArt {
+                clientController.getCover(id: coverID)
+            }
         }
NattyNarwhal commented 7 months ago

That works; it might also be a good idea to also perhaps batch the requests for album/artist/cover by ID and do it at the end. Since they spawn a new operation regardless of when it's called, I don't think it would affect the logic in the code.

NattyNarwhal commented 7 months ago

Something like:

diff --git a/Submariner/SBSubsonicParsingOperation.swift b/Submariner/SBSubsonicParsingOperation.swift
index aca281a..19fd404 100644
--- a/Submariner/SBSubsonicParsingOperation.swift
+++ b/Submariner/SBSubsonicParsingOperation.swift
@@ -86,6 +86,9 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
     var currentAlbumID: String?
     var currentCoverID: String?

+    // used to batch requests, i.e. covers in a playlist
+    var getCoverIDs = Set<String>()
+    
     init!(managedObjectContext mainContext: NSManagedObjectContext!,
           client: SBClientController,
           requestType: RequestType,
@@ -287,7 +290,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                (cover.imagePath == nil || !FileManager.default.fileExists(atPath: cover.imagePath! as String)) {
                 // file doesn't exist, fetch it
                 logger.info("Fetching file for cover with ID: \(coverID, privacy: .public)")
-                clientController.getCover(id: coverID)
+                getCoverIDs.insert(coverID)
             }
         }
     }
@@ -361,7 +364,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 }

                 if album?.cover?.imagePath == nil {
-                    clientController.getCover(id: coverArt)
+                    getCoverIDs.insert(coverArt)
                 }
             }
         }
@@ -627,6 +630,9 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
         threadedContext.processPendingChanges()
         saveThreadedContext()

+        // submit further requests. since they run in their own context, it doesn't matter that we submit after updating Core Data
+        getCoverIDs.forEach { self.clientController.getCover(id: $0) }
+        
         if requestType == .ping && !errored {
             postServerNotification(.SBSubsonicConnectionSucceeded)
         } else if requestType == .updatePlaylist {
@@ -874,7 +880,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 attachedAlbum.cover = attachedCover!
             }

-            clientController.getCover(id: coverID)
+            getCoverIDs.insert(coverID)
         }

         if let attachedAlbum = attachedAlbum {
@@ -925,7 +931,7 @@ class SBSubsonicParsingOperation: SBOperation, XMLParserDelegate {
                 attachedAlbum.cover = attachedCover!
             }

-            clientController.getCover(id: coverID)
+            getCoverIDs.insert(coverID)
         }

         if let attachedAlbum = attachedAlbum, let artistName = attributeDict["artist"] {

Unfortunately, I think Navidrome provides a separate cover ID for each track, so this doesn't actually save anything over your solution.

NattyNarwhal commented 7 months ago

I've applied your patch and merged it.