Merge branch 'feature/fix-double-read'

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) {

Modules/Account/Sources/Account/FeedFinder/FeedFinder.swift

@@ -14,48 +14,50 @@ import RSCore
 class FeedFinder {
 	
 	static func find(url: URL, completion: @escaping (Result<Set<FeedSpecifier>, 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) {

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)
+				}
 			}
 		}
 	}

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<DownloaderRecord>(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?
+}

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)")
 			}
 		}
 	}

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)
 		}
 	}
 

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)
 		}
 	}