From 248fe49af48443a68fb703743375cae2ea87261f Mon Sep 17 00:00:00 2001 From: Brent Simmons Date: Tue, 26 Nov 2024 22:03:23 -0800 Subject: [PATCH] Simplify local feed downloading. --- .../CloudKit/CloudKitAccountDelegate.swift | 12 +-- .../LocalAccount/LocalAccountDelegate.swift | 11 +-- .../LocalAccount/LocalAccountRefresher.swift | 95 ++++++------------- RSWeb/Sources/RSWeb/DownloadSession.swift | 90 ++++++------------ 4 files changed, 62 insertions(+), 146 deletions(-) diff --git a/Account/Sources/Account/CloudKit/CloudKitAccountDelegate.swift b/Account/Sources/Account/CloudKit/CloudKitAccountDelegate.swift index 04c84723c..12d7bad13 100644 --- a/Account/Sources/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Account/Sources/Account/CloudKit/CloudKitAccountDelegate.swift @@ -713,7 +713,7 @@ private extension CloudKitAccountDelegate { } } - func storeArticleChanges(new: Set
?, updated: Set
?, deleted: Set
?, completion: @escaping () -> Void) { + func storeArticleChanges(new: Set
?, updated: Set
?, deleted: Set
?, completion: (() -> Void)?) { // New records with a read status aren't really new, they just didn't have the read article stored let group = DispatchGroup() if let new = new { @@ -736,7 +736,7 @@ private extension CloudKitAccountDelegate { group.notify(queue: DispatchQueue.global(qos: .userInitiated)) { DispatchQueue.main.async { - completion() + completion?() } } } @@ -798,15 +798,11 @@ private extension CloudKitAccountDelegate { extension CloudKitAccountDelegate: LocalAccountRefresherDelegate { - func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { - refreshProgress.completeTask() - } - - func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges, completion: @escaping () -> Void) { + func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges) { self.storeArticleChanges(new: articleChanges.newArticles, updated: articleChanges.updatedArticles, deleted: articleChanges.deletedArticles, - completion: completion) + completion: nil) } } diff --git a/Account/Sources/Account/LocalAccount/LocalAccountDelegate.swift b/Account/Sources/Account/LocalAccount/LocalAccountDelegate.swift index 74b2fd6f8..28a2a41d2 100644 --- a/Account/Sources/Account/LocalAccount/LocalAccountDelegate.swift +++ b/Account/Sources/Account/LocalAccount/LocalAccountDelegate.swift @@ -217,16 +217,9 @@ final class LocalAccountDelegate: AccountDelegate { } extension LocalAccountDelegate: LocalAccountRefresherDelegate { - - - func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { - refreshProgress.completeTask() + + func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges) { } - - func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges, completion: @escaping () -> Void) { - completion() - } - } private extension LocalAccountDelegate { diff --git a/Account/Sources/Account/LocalAccount/LocalAccountRefresher.swift b/Account/Sources/Account/LocalAccount/LocalAccountRefresher.swift index 9b8dda747..98223fa81 100644 --- a/Account/Sources/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Account/Sources/Account/LocalAccount/LocalAccountRefresher.swift @@ -14,8 +14,7 @@ import Articles import ArticlesDatabase protocol LocalAccountRefresherDelegate { - func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) - func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges, completion: @escaping () -> Void) + func localAccountRefresher(_ refresher: LocalAccountRefresher, articleChanges: ArticleChanges) } final class LocalAccountRefresher { @@ -23,18 +22,30 @@ final class LocalAccountRefresher { private var completion: (() -> Void)? = nil private var isSuspended = false var delegate: LocalAccountRefresherDelegate? - + private lazy var downloadSession: DownloadSession = { return DownloadSession(delegate: self) }() + private var urlToFeedDictionary = [String: WebFeed]() + public func refreshFeeds(_ feeds: Set, completion: (() -> Void)? = nil) { guard !feeds.isEmpty else { completion?() return } + + urlToFeedDictionary.removeAll() + for feed in feeds { + urlToFeedDictionary[feed.url] = feed + } + + let urls = feeds.compactMap { feed in + URL(unicodeString: feed.url) + } + self.completion = completion - downloadSession.downloadObjects(feeds as NSSet) + downloadSession.download(Set(urls)) } public func suspend() { @@ -51,100 +62,50 @@ final class LocalAccountRefresher { extension LocalAccountRefresher: DownloadSessionDelegate { - func downloadSession(_ downloadSession: DownloadSession, requestForRepresentedObject representedObject: AnyObject) -> URLRequest? { - guard let feed = representedObject as? WebFeed else { - return nil - } - guard let url = URL(string: feed.url) else { - return nil - } + func downloadSession(_ downloadSession: DownloadSession, downloadDidComplete url: URL, response: URLResponse?, data: Data, error: NSError?) { - return URLRequest(url: url) - } - - func downloadSession(_ downloadSession: DownloadSession, downloadDidCompleteForRepresentedObject representedObject: AnyObject, response: URLResponse?, data: Data, error: NSError?, completion: @escaping () -> Void) { - let feed = representedObject as! WebFeed - + guard let feed = urlToFeedDictionary[url.absoluteString] else { + return + } guard !data.isEmpty, !isSuspended else { - completion() - delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } - if let error = error { - print("Error downloading \(feed.url) - \(error)") - completion() - delegate?.localAccountRefresher(self, requestCompletedFor: feed) + if let error { + print("Error downloading \(url) - \(error)") return } let dataHash = data.md5String if dataHash == feed.contentHash { - completion() - delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } let parserData = ParserData(url: feed.url, data: data) FeedParser.parse(parserData) { (parsedFeed, error) in - guard let account = feed.account, let parsedFeed = parsedFeed, error == nil else { - completion() - self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) + guard let account = feed.account, let parsedFeed, error == nil else { return } account.update(feed, with: parsedFeed) { result in if case .success(let articleChanges) = result { feed.contentHash = dataHash - self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) - self.delegate?.localAccountRefresher(self, articleChanges: articleChanges) { - completion() - } - } else { - completion() - self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) + self.delegate?.localAccountRefresher(self, articleChanges: articleChanges) } } - } } - func downloadSession(_ downloadSession: DownloadSession, shouldContinueAfterReceivingData data: Data, representedObject: AnyObject) -> Bool { - let feed = representedObject as! WebFeed - guard !isSuspended else { - delegate?.localAccountRefresher(self, requestCompletedFor: feed) + func downloadSession(_ downloadSession: DownloadSession, shouldContinueAfterReceivingData data: Data, url: URL) -> Bool { + + guard !data.isDefinitelyNotFeed(), !isSuspended else { return false } - - if data.isEmpty { - return true - } - - if data.isDefinitelyNotFeed() { - delegate?.localAccountRefresher(self, requestCompletedFor: feed) - return false - } - - return true - } - - func downloadSession(_ downloadSession: DownloadSession, didReceiveUnexpectedResponse response: URLResponse, representedObject: AnyObject) { - let feed = representedObject as! WebFeed - delegate?.localAccountRefresher(self, requestCompletedFor: feed) - } - - func downloadSession(_ downloadSession: DownloadSession, didReceiveNotModifiedResponse: URLResponse, representedObject: AnyObject) { - let feed = representedObject as! WebFeed - delegate?.localAccountRefresher(self, requestCompletedFor: feed) + return true } - func downloadSession(_ downloadSession: DownloadSession, didDiscardDuplicateRepresentedObject representedObject: AnyObject) { - let feed = representedObject as! WebFeed - delegate?.localAccountRefresher(self, requestCompletedFor: feed) - } - - func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) { + func downloadSessionDidComplete(_ downloadSession: DownloadSession) { completion?() completion = nil } diff --git a/RSWeb/Sources/RSWeb/DownloadSession.swift b/RSWeb/Sources/RSWeb/DownloadSession.swift index 2ad411ef3..541760ebe 100755 --- a/RSWeb/Sources/RSWeb/DownloadSession.swift +++ b/RSWeb/Sources/RSWeb/DownloadSession.swift @@ -13,14 +13,9 @@ import Foundation public protocol DownloadSessionDelegate { - func downloadSession(_ downloadSession: DownloadSession, requestForRepresentedObject: AnyObject) -> URLRequest? - func downloadSession(_ downloadSession: DownloadSession, downloadDidCompleteForRepresentedObject: AnyObject, response: URLResponse?, data: Data, error: NSError?, completion: @escaping () -> Void) - func downloadSession(_ downloadSession: DownloadSession, shouldContinueAfterReceivingData: Data, representedObject: AnyObject) -> Bool - func downloadSession(_ downloadSession: DownloadSession, didReceiveUnexpectedResponse: URLResponse, representedObject: AnyObject) - func downloadSession(_ downloadSession: DownloadSession, didReceiveNotModifiedResponse: URLResponse, representedObject: AnyObject) - func downloadSession(_ downloadSession: DownloadSession, didDiscardDuplicateRepresentedObject: AnyObject) - func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) - + func downloadSession(_ downloadSession: DownloadSession, downloadDidComplete: URL, response: URLResponse?, data: Data, error: NSError?) + func downloadSession(_ downloadSession: DownloadSession, shouldContinueAfterReceivingData: Data, url: URL) -> Bool + func downloadSessionDidComplete(_ downloadSession: DownloadSession) } @objc public final class DownloadSession: NSObject { @@ -29,10 +24,10 @@ public protocol DownloadSessionDelegate { private var tasksInProgress = Set() private var tasksPending = Set() private var taskIdentifierToInfoDictionary = [Int: DownloadInfo]() - private let representedObjects = NSMutableSet() + private var urlsInSession = Set() private let delegate: DownloadSessionDelegate private var redirectCache = [String: String]() - private var queue = [AnyObject]() + private var queue = [URL]() private var retryAfterMessages = [String: HTTPResponse429]() public init(delegate: DownloadSessionDelegate) { @@ -70,15 +65,12 @@ public protocol DownloadSessionDelegate { } } - public func downloadObjects(_ objects: NSSet) { - for oneObject in objects { - if !representedObjects.contains(oneObject) { - representedObjects.add(oneObject) - addDataTask(oneObject as AnyObject) - } else { - delegate.downloadSession(self, didDiscardDuplicateRepresentedObject: oneObject as AnyObject) - } + public func download(_ urls: Set) { + + for url in urls.subtracting(urlsInSession) { + addDataTask(url) } + urlsInSession.formUnion(urls) } } @@ -90,16 +82,13 @@ extension DownloadSession: URLSessionTaskDelegate { tasksInProgress.remove(task) guard let info = infoForTask(task) else { + removeTask(task) return } - - info.error = error - delegate.downloadSession(self, downloadDidCompleteForRepresentedObject: info.representedObject, response: info.urlResponse, data: info.data as Data, error: error as NSError?) { - self.removeTask(task) - } + delegate.downloadSession(self, downloadDidComplete: info.url, response: info.urlResponse, data: info.data as Data, error: error as NSError?) } - + public func urlSession(_ session: URLSession, task: URLSessionTask, willPerformHTTPRedirection response: HTTPURLResponse, newRequest request: URLRequest, completionHandler: @escaping (URLRequest?) -> Void) { if response.statusCode == 301 || response.statusCode == 308 { @@ -124,30 +113,13 @@ extension DownloadSession: URLSessionDataDelegate { if let info = infoForTask(dataTask) { info.urlResponse = response } - let statusCode = response.forcedStatusCode - - if statusCode == HTTPResponseCode.notModified { - - if let representedObject = infoForTask(dataTask)?.representedObject { - delegate.downloadSession(self, didReceiveNotModifiedResponse: response, representedObject: representedObject) - } - - completionHandler(.cancel) - removeTask(dataTask) - - return - } if !response.statusIsOK { - if let representedObject = infoForTask(dataTask)?.representedObject { - delegate.downloadSession(self, didReceiveUnexpectedResponse: response, representedObject: representedObject) - } - completionHandler(.cancel) removeTask(dataTask) - if statusCode == HTTPResponseCode.tooManyRequests { + if response.forcedStatusCode == HTTPResponseCode.tooManyRequests { handle429Response(dataTask, response) } @@ -155,7 +127,6 @@ extension DownloadSession: URLSessionDataDelegate { } addDataTaskFromQueueIfNecessary() - completionHandler(.allow) } @@ -166,8 +137,8 @@ extension DownloadSession: URLSessionDataDelegate { } info.addData(data) - if !delegate.downloadSession(self, shouldContinueAfterReceivingData: info.data as Data, representedObject: info.representedObject) { - + if !delegate.downloadSession(self, shouldContinueAfterReceivingData: info.data as Data, url: info.url) { + info.canceled = true dataTask.cancel() removeTask(dataTask) @@ -179,21 +150,19 @@ extension DownloadSession: URLSessionDataDelegate { private extension DownloadSession { - func addDataTask(_ representedObject: AnyObject) { + func addDataTask(_ url: URL) { guard tasksPending.count < 500 else { - queue.insert(representedObject, at: 0) - return - } - - guard let request = delegate.downloadSession(self, requestForRepresentedObject: representedObject) else { + queue.insert(url, at: 0) return } + let request = URLRequest(url: url) var requestToUse = request // If received permanent redirect earlier, use that URL. - if let urlString = request.url?.absoluteString, let redirectedURLString = cachedRedirectForURLString(urlString) { + let urlString = url.absoluteString + if let redirectedURLString = cachedRedirectForURLString(urlString) { if let redirectedURL = URL(string: redirectedURLString) { requestToUse.url = redirectedURL } @@ -205,7 +174,7 @@ private extension DownloadSession { let task = urlSession.dataTask(with: requestToUse) - let info = DownloadInfo(representedObject, urlRequest: requestToUse) + let info = DownloadInfo(url) taskIdentifierToInfoDictionary[task.taskIdentifier] = info tasksPending.insert(task) @@ -229,8 +198,8 @@ private extension DownloadSession { addDataTaskFromQueueIfNecessary() if tasksInProgress.count + tasksPending.count < 1 { - representedObjects.removeAllObjects() - delegate.downloadSessionDidCompleteDownloadObjects(self) + urlsInSession.removeAll() + delegate.downloadSessionDidComplete(self) } } @@ -373,10 +342,8 @@ extension URLSessionTask { private final class DownloadInfo { - let representedObject: AnyObject - let urlRequest: URLRequest + let url: URL let data = NSMutableData() - var error: Error? var urlResponse: URLResponse? var canceled = false @@ -384,10 +351,9 @@ private final class DownloadInfo { return urlResponse?.forcedStatusCode ?? 0 } - init(_ representedObject: AnyObject, urlRequest: URLRequest) { - - self.representedObject = representedObject - self.urlRequest = urlRequest + init(_ url: URL) { + + self.url = url } func addData(_ d: Data) {