diff --git a/Mac/CrashReporter/CrashReporter.swift b/Mac/CrashReporter/CrashReporter.swift index 95423994d..e258fabc7 100644 --- a/Mac/CrashReporter/CrashReporter.swift +++ b/Mac/CrashReporter/CrashReporter.swift @@ -54,7 +54,9 @@ struct CrashReporter { let formData = formString.data(using: .utf8, allowLossyConversion: true) request.httpBody = formData - Downloader.shared.download(request) // Don’t care about the result. + Task { @MainActor in + Downloader.shared.download(request) { _, _, _ in } + } } static func runCrashReporterWindow(_ crashLogText: String) { diff --git a/Modules/Account/Sources/Account/FeedFinder/FeedFinder.swift b/Modules/Account/Sources/Account/FeedFinder/FeedFinder.swift index 5a9084acb..643173e28 100644 --- a/Modules/Account/Sources/Account/FeedFinder/FeedFinder.swift +++ b/Modules/Account/Sources/Account/FeedFinder/FeedFinder.swift @@ -14,48 +14,50 @@ import RSCore class FeedFinder { static func find(url: URL, completion: @escaping (Result, Error>) -> Void) { - Downloader.shared.download(url) { (data, response, error) in + Task { @MainActor in + Downloader.shared.download(url) { (data, response, error) in - if response?.forcedStatusCode == 404 { - if var urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false), urlComponents.host == "micro.blog" { - urlComponents.path = "\(urlComponents.path).json" - if let newURLString = urlComponents.url?.absoluteString { - let microblogFeedSpecifier = FeedSpecifier(title: nil, urlString: newURLString, source: .HTMLLink, orderFound: 1) - completion(.success(Set([microblogFeedSpecifier]))) + if response?.forcedStatusCode == 404 { + if var urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false), urlComponents.host == "micro.blog" { + urlComponents.path = "\(urlComponents.path).json" + if let newURLString = urlComponents.url?.absoluteString { + let microblogFeedSpecifier = FeedSpecifier(title: nil, urlString: newURLString, source: .HTMLLink, orderFound: 1) + completion(.success(Set([microblogFeedSpecifier]))) + } + } else { + completion(.failure(AccountError.createErrorNotFound)) } - } else { - completion(.failure(AccountError.createErrorNotFound)) + return } - return + + if let error = error { + completion(.failure(error)) + return + } + + guard let data = data, let response = response else { + completion(.failure(AccountError.createErrorNotFound)) + return + } + + if !response.statusIsOK || data.isEmpty { + completion(.failure(AccountError.createErrorNotFound)) + return + } + + if FeedFinder.isFeed(data, url.absoluteString) { + let feedSpecifier = FeedSpecifier(title: nil, urlString: url.absoluteString, source: .UserEntered, orderFound: 1) + completion(.success(Set([feedSpecifier]))) + return + } + + if !FeedFinder.isHTML(data) { + completion(.failure(AccountError.createErrorNotFound)) + return + } + + FeedFinder.findFeedsInHTMLPage(htmlData: data, urlString: url.absoluteString, completion: completion) } - - if let error = error { - completion(.failure(error)) - return - } - - guard let data = data, let response = response else { - completion(.failure(AccountError.createErrorNotFound)) - return - } - - if !response.statusIsOK || data.isEmpty { - completion(.failure(AccountError.createErrorNotFound)) - return - } - - if FeedFinder.isFeed(data, url.absoluteString) { - let feedSpecifier = FeedSpecifier(title: nil, urlString: url.absoluteString, source: .UserEntered, orderFound: 1) - completion(.success(Set([feedSpecifier]))) - return - } - - if !FeedFinder.isHTML(data) { - completion(.failure(AccountError.createErrorNotFound)) - return - } - - FeedFinder.findFeedsInHTMLPage(htmlData: data, urlString: url.absoluteString, completion: completion) } } } @@ -140,22 +142,23 @@ private extension FeedFinder { var resultFeedSpecifiers = feedSpecifiers let group = DispatchGroup() - + for downloadFeedSpecifier in downloadFeedSpecifiers { guard let url = URL(string: downloadFeedSpecifier.urlString) else { continue } - + group.enter() - Downloader.shared.download(url) { (data, response, error) in - if let data = data, let response = response, response.statusIsOK, error == nil { - if self.isFeed(data, downloadFeedSpecifier.urlString) { - addFeedSpecifier(downloadFeedSpecifier, feedSpecifiers: &resultFeedSpecifiers) + Task { @MainActor in + Downloader.shared.download(url) { (data, response, error) in + if let data = data, let response = response, response.statusIsOK, error == nil { + if self.isFeed(data, downloadFeedSpecifier.urlString) { + addFeedSpecifier(downloadFeedSpecifier, feedSpecifiers: &resultFeedSpecifiers) + } } + group.leave() } - group.leave() } - } group.notify(queue: DispatchQueue.main) { diff --git a/Modules/Account/Sources/Account/LocalAccount/InitialFeedDownloader.swift b/Modules/Account/Sources/Account/LocalAccount/InitialFeedDownloader.swift index d7ac118ed..606a740ca 100644 --- a/Modules/Account/Sources/Account/LocalAccount/InitialFeedDownloader.swift +++ b/Modules/Account/Sources/Account/LocalAccount/InitialFeedDownloader.swift @@ -13,16 +13,17 @@ import RSWeb struct InitialFeedDownloader { static func download(_ url: URL,_ completion: @escaping (_ parsedFeed: ParsedFeed?) -> Void) { - - Downloader.shared.download(url) { (data, response, error) in - guard let data = data else { - completion(nil) - return - } - - let parserData = ParserData(url: url.absoluteString, data: data) - FeedParser.parse(parserData) { (parsedFeed, error) in - completion(parsedFeed) + Task { @MainActor in + Downloader.shared.download(url) { (data, response, error) in + guard let data = data else { + completion(nil) + return + } + + let parserData = ParserData(url: url.absoluteString, data: data) + FeedParser.parse(parserData) { (parsedFeed, error) in + completion(parsedFeed) + } } } } diff --git a/Modules/RSWeb/Sources/RSWeb/Downloader.swift b/Modules/RSWeb/Sources/RSWeb/Downloader.swift index c0dc12ca8..ecdab5abc 100755 --- a/Modules/RSWeb/Sources/RSWeb/Downloader.swift +++ b/Modules/RSWeb/Sources/RSWeb/Downloader.swift @@ -7,18 +7,27 @@ // import Foundation +import os +import RSCore -public typealias DownloadCallback = (Data?, URLResponse?, Error?) -> Swift.Void +public typealias DownloadCallback = @MainActor (Data?, URLResponse?, Error?) -> Swift.Void /// Simple downloader, for a one-shot download like an image /// or a web page. For a download-feeds session, see DownloadSession. -public final class Downloader { +/// Caches response for a short time for GET requests. May return cached response. +@MainActor public final class Downloader { public static let shared = Downloader() private let urlSession: URLSession + private var callbacks = [URL: [DownloadCallback]]() + + // Cache — short-lived + private let cache = Cache(timeToLive: 60 * 3, timeBetweenCleanups: 60 * 2) + + nonisolated private static let logger = Logger(subsystem: Bundle.main.bundleIdentifier!, category: "Downloader") + nonisolated private static let debugLoggingEnabled = true private init() { - let sessionConfiguration = URLSessionConfiguration.ephemeral sessionConfiguration.requestCachePolicy = .reloadIgnoringLocalCacheData sessionConfiguration.httpShouldSetCookies = false @@ -37,20 +46,103 @@ public final class Downloader { urlSession.invalidateAndCancel() } - public func download(_ url: URL, _ completion: DownloadCallback? = nil) { - download(URLRequest(url: url), completion) + public func download(_ url: URL, _ callback: @escaping DownloadCallback) { + assert(Thread.isMainThread) + download(URLRequest(url: url), callback) } - public func download(_ urlRequest: URLRequest, _ completion: DownloadCallback? = nil) { + public func download(_ urlRequest: URLRequest, _ callback: @escaping DownloadCallback) { + assert(Thread.isMainThread) + + guard let url = urlRequest.url else { + Self.logger.fault("Downloader: skipping download for URLRequest without a URL") + return + } + + let isCacheableRequest = urlRequest.httpMethod == HTTPMethod.get + + // Return cached record if available. + if isCacheableRequest { + if let cachedRecord = cache[url.absoluteString] { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: returning cached record for \(url)") + } + callback(cachedRecord.data, cachedRecord.response, cachedRecord.error) + return + } + } + + // Add callback. If there is already a download in progress for this URL, return early. + if callbacks[url] == nil { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: downloading \(url)") + } + callbacks[url] = [callback] + } else { + // A download is already be in progress for this URL. Don’t start a separate download. + // Add the callback to the callbacks array for this URL. + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: download in progress for \(url) — adding callback") + } + callbacks[url]?.append(callback) + return + } var urlRequestToUse = urlRequest urlRequestToUse.addSpecialCaseUserAgentIfNeeded() let task = urlSession.dataTask(with: urlRequestToUse) { (data, response, error) in - DispatchQueue.main.async() { - completion?(data, response, error) + + if isCacheableRequest { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: caching record for \(url)") + } + let cachedRecord = DownloaderRecord(data: data, response: response, error: error) + self.cache[url.absoluteString] = cachedRecord + } + + Task { @MainActor in + self.callAndReleaseCallbacks(url, data, response, error) } } task.resume() } } + +private extension Downloader { + + func callAndReleaseCallbacks(_ url: URL, _ data: Data? = nil, _ response: URLResponse? = nil, _ error: Error? = nil) { + assert(Thread.isMainThread) + + defer { + callbacks[url] = nil + } + + guard let callbacksForURL = callbacks[url] else { + assertionFailure("Downloader: downloaded URL \(url) but no callbacks found") + Self.logger.fault("Downloader: downloaded URL \(url) but no callbacks found") + return + } + + if Self.debugLoggingEnabled { + let count = callbacksForURL.count + if count == 1 { + Self.logger.debug("Downloader: calling 1 callback for URL \(url)") + } else { + Self.logger.debug("Downloader: calling \(count) callbacks for URL \(url)") + } + } + + for callback in callbacksForURL { + callback(data, response, error) + } + } +} + +struct DownloaderRecord: CacheRecord, Sendable { + + let dateCreated = Date() + let data: Data? + let response: URLResponse? + let error: Error? +} diff --git a/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift b/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift index c59489aeb..6d8a0eea4 100644 --- a/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift +++ b/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift @@ -87,24 +87,26 @@ private extension HTMLMetadataDownloader { Self.logger.debug("HTMLMetadataDownloader downloading for \(url)") } - Downloader.shared.download(actualURL) { data, response, error in - if let data, !data.isEmpty, let response, response.statusIsOK { - let urlToUse = response.url ?? actualURL - let parserData = ParserData(url: urlToUse.absoluteString, data: data) - let htmlMetadata = RSHTMLMetadataParser.htmlMetadata(with: parserData) - if Self.debugLoggingEnabled { - Self.logger.debug("HTMLMetadataDownloader caching parsed metadata for \(url)") + Task { @MainActor in + Downloader.shared.download(actualURL) { data, response, error in + if let data, !data.isEmpty, let response, response.statusIsOK { + let urlToUse = response.url ?? actualURL + let parserData = ParserData(url: urlToUse.absoluteString, data: data) + let htmlMetadata = RSHTMLMetadataParser.htmlMetadata(with: parserData) + if Self.debugLoggingEnabled { + Self.logger.debug("HTMLMetadataDownloader caching parsed metadata for \(url)") + } + self.cache[url] = htmlMetadata + return + } + + if let statusCode = response?.forcedStatusCode, (400...499).contains(statusCode) { + self.noteURLDidReturn4xx(url) + } + + if Self.debugLoggingEnabled { + Self.logger.debug("HTMLMetadataDownloader failed download for \(url)") } - self.cache[url] = htmlMetadata - return - } - - if let statusCode = response?.forcedStatusCode, (400...499).contains(statusCode) { - self.noteURLDidReturn4xx(url) - } - - if Self.debugLoggingEnabled { - Self.logger.debug("HTMLMetadataDownloader failed download for \(url)") } } } diff --git a/Shared/Favicons/SingleFaviconDownloader.swift b/Shared/Favicons/SingleFaviconDownloader.swift index 67e30dc19..bc952bdf4 100644 --- a/Shared/Favicons/SingleFaviconDownloader.swift +++ b/Shared/Favicons/SingleFaviconDownloader.swift @@ -139,19 +139,21 @@ private extension SingleFaviconDownloader { return } - Downloader.shared.download(url) { (data, response, error) in - - if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { - self.saveToDisk(data) - RSImage.image(with: data, imageResultBlock: completion) - return + Task { @MainActor in + Downloader.shared.download(url) { (data, response, error) in + + if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { + self.saveToDisk(data) + RSImage.image(with: data, imageResultBlock: completion) + return + } + + if let error = error { + os_log(.info, log: self.log, "Error downloading image at %@: %@.", url.absoluteString, error.localizedDescription) + } + + completion(nil) } - - if let error = error { - os_log(.info, log: self.log, "Error downloading image at %@: %@.", url.absoluteString, error.localizedDescription) - } - - completion(nil) } } diff --git a/Shared/Images/ImageDownloader.swift b/Shared/Images/ImageDownloader.swift index fcafe8e1f..b5233ae55 100644 --- a/Shared/Images/ImageDownloader.swift +++ b/Shared/Images/ImageDownloader.swift @@ -105,22 +105,24 @@ private extension ImageDownloader { return } - Downloader.shared.download(imageURL) { (data, response, error) in - - if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { - self.saveToDisk(url, data) - completion(data) - return + Task { @MainActor in + Downloader.shared.download(imageURL) { (data, response, error) in + + if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { + self.saveToDisk(url, data) + completion(data) + return + } + + if let response = response as? HTTPURLResponse, response.statusCode >= HTTPResponseCode.badRequest && response.statusCode <= HTTPResponseCode.notAcceptable { + self.badURLs.insert(url) + } + if let error = error { + os_log(.info, log: self.log, "Error downloading image at %@: %@.", url, error.localizedDescription) + } + + completion(nil) } - - if let response = response as? HTTPURLResponse, response.statusCode >= HTTPResponseCode.badRequest && response.statusCode <= HTTPResponseCode.notAcceptable { - self.badURLs.insert(url) - } - if let error = error { - os_log(.info, log: self.log, "Error downloading image at %@: %@.", url, error.localizedDescription) - } - - completion(nil) } }