diff --git a/Sources/Caching/LargeItemCacheType.swift b/Sources/Caching/LargeItemCacheType.swift index 5585c09ca0..965f4a5634 100644 --- a/Sources/Caching/LargeItemCacheType.swift +++ b/Sources/Caching/LargeItemCacheType.swift @@ -25,6 +25,9 @@ protocol LargeItemCacheType { /// Check if there is content cached at the url func cachedContentExists(at url: URL) -> Bool + /// Check if there is a regular file cached at the url + func cachedFileExists(at url: URL) -> Bool + /// Load data from url func loadFile(at url: URL) throws -> Data @@ -151,6 +154,11 @@ extension FileManager: LargeItemCacheType { } } + /// Check if a regular file is cached at the given path + func cachedFileExists(at url: URL) -> Bool { + return (try? url.resourceValues(forKeys: [.isRegularFileKey]).isRegularFile) == true + } + /// Creates a directory from a base path in the specified directory type /// The `inAppSpecificDirectory` should be set to false only for components /// that haven't migrated to the new app specific directory structure yet diff --git a/Sources/Networking/RemoteConfigBlobStore.swift b/Sources/Networking/RemoteConfigBlobStore.swift index 755e7646ac..3625b2fc7a 100644 --- a/Sources/Networking/RemoteConfigBlobStore.swift +++ b/Sources/Networking/RemoteConfigBlobStore.swift @@ -23,7 +23,7 @@ protocol RemoteConfigBlobStoreType: AnyObject { /// Content-addressed disk cache for remote config blobs, keyed by 32-character URL-safe base64 refs. final class RemoteConfigBlobStore: RemoteConfigBlobStoreType { - private let fileManager: FileManager + private let cache: LargeItemCacheType private let directoryURL: URL? private let lock = Lock(.nonRecursive) @@ -32,10 +32,10 @@ final class RemoteConfigBlobStore: RemoteConfigBlobStoreType { private var knownRefs: Set? init( - fileManager: FileManager = .default, + cache: LargeItemCacheType = FileManager.default, directoryURL: URL? = RemoteConfigBlobStore.defaultDirectoryURL ) { - self.fileManager = fileManager + self.cache = cache self.directoryURL = directoryURL } @@ -86,7 +86,7 @@ private extension RemoteConfigBlobStore { func containsWithoutLock(ref: String) -> Bool { guard self.loadedRefsWithoutLock().contains(ref), let fileURL = self.fileURL(for: ref), - self.isRegularFile(fileURL) else { + self.cache.cachedFileExists(at: fileURL) else { self.knownRefs?.remove(ref) return false } @@ -96,13 +96,13 @@ private extension RemoteConfigBlobStore { func readWithoutLock(ref: String) -> Data? { guard let fileURL = self.fileURL(for: ref), - self.isRegularFile(fileURL) else { + self.cache.cachedFileExists(at: fileURL) else { self.knownRefs?.remove(ref) return nil } do { - return try Data(contentsOf: fileURL) + return try self.cache.loadFile(at: fileURL) } catch { Logger.error(Strings.remoteConfig.failedToReadBlob(ref, error)) return nil @@ -113,7 +113,7 @@ private extension RemoteConfigBlobStore { ref: String, bytes: UnsafeRawBufferPointer ) -> Bool { - guard let directoryURL = self.directoryURL else { + guard self.directoryURL != nil else { Logger.error(Strings.remoteConfig.cacheURLNotAvailable) return false } @@ -124,15 +124,9 @@ private extension RemoteConfigBlobStore { } do { - try self.fileManager.createDirectory( - at: directoryURL, - withIntermediateDirectories: true, - attributes: nil - ) - var data = Data() data.append(contentsOf: bytes.bindMemory(to: UInt8.self)) - try data.write(to: fileURL, options: .atomic) + try self.cache.saveData(data, to: fileURL) guard var refs = self.knownRefs else { return true } @@ -155,18 +149,15 @@ private extension RemoteConfigBlobStore { return } - guard let contents = try? self.fileManager.contentsOfDirectory( - at: directoryURL, - includingPropertiesForKeys: [.isRegularFileKey], - options: [] - ) else { + guard let contents = try? self.cache.contentsOfDirectory(at: directoryURL) else { self.knownRefs = nil return } - for fileURL in contents where self.isRegularFile(fileURL) && !validRefs.contains(fileURL.lastPathComponent) { + for fileURL in contents where self.cache.cachedFileExists(at: fileURL) + && !validRefs.contains(fileURL.lastPathComponent) { do { - try self.fileManager.removeItem(at: fileURL) + try self.cache.remove(fileURL) } catch { Logger.error(Strings.remoteConfig.failedToDeleteBlob(fileURL.lastPathComponent, error)) } @@ -176,16 +167,20 @@ private extension RemoteConfigBlobStore { } func clearWithoutLock() { - guard let directoryURL = self.directoryURL, - self.fileManager.fileExists(atPath: directoryURL.path) else { + guard let directoryURL = self.directoryURL else { self.knownRefs = [] return } do { - try self.fileManager.removeItem(at: directoryURL) + try self.cache.remove(directoryURL) self.knownRefs = [] } catch { + guard !self.isMissingFileError(error) else { + self.knownRefs = [] + return + } + Logger.error(Strings.remoteConfig.failedToClearBlobStore(error)) self.knownRefs = nil } @@ -206,8 +201,9 @@ private extension RemoteConfigBlobStore { return self.directoryURL?.appendingPathComponent(ref, isDirectory: false) } - func isRegularFile(_ fileURL: URL) -> Bool { - return (try? fileURL.resourceValues(forKeys: [.isRegularFileKey]).isRegularFile) == true + func isMissingFileError(_ error: Error) -> Bool { + let error = error as NSError + return error.domain == NSCocoaErrorDomain && error.code == CocoaError.fileNoSuchFile.rawValue } func loadedRefsWithoutLock() -> Set { @@ -230,16 +226,12 @@ private extension RemoteConfigBlobStore { func scannedRefsWithoutLock() -> Set? { guard let directoryURL = self.directoryURL, - let contents = try? self.fileManager.contentsOfDirectory( - at: directoryURL, - includingPropertiesForKeys: [.isRegularFileKey], - options: [] - ) else { + let contents = try? self.cache.contentsOfDirectory(at: directoryURL) else { return nil } let scannedRefs = contents.reduce(into: Set()) { refs, fileURL in - guard self.isRegularFile(fileURL), + guard self.cache.cachedFileExists(at: fileURL), RemoteConfigBlobRefHelpers.isValid(fileURL.lastPathComponent) else { return } diff --git a/Tests/UnitTests/Caching/LargeItemCacheTypeTests.swift b/Tests/UnitTests/Caching/LargeItemCacheTypeTests.swift index bea547010c..e6ec46790a 100644 --- a/Tests/UnitTests/Caching/LargeItemCacheTypeTests.swift +++ b/Tests/UnitTests/Caching/LargeItemCacheTypeTests.swift @@ -216,6 +216,46 @@ final class LargeItemCacheTypeTests: TestCase { } + // MARK: - cachedFileExists Tests + + func testCachedFileExistsReturnsTrueForNonEmptyFile() throws { + let url = self.testDirectory.appendingPathComponent("cached_file_nonempty.txt") + try "A".write(to: url, atomically: true, encoding: .utf8) + + let exists = self.fileManager.cachedFileExists(at: url) + + expect(exists) == true + try fileManager.removeItem(at: url) + } + + func testCachedFileExistsReturnsTrueForEmptyFile() throws { + let url = self.testDirectory.appendingPathComponent("cached_file_empty.txt") + try Data().write(to: url) + + let exists = self.fileManager.cachedFileExists(at: url) + + expect(exists) == true + try fileManager.removeItem(at: url) + } + + func testCachedFileExistsReturnsFalseForDirectory() throws { + let url = self.testDirectory.appendingPathComponent("cached_file_directory", isDirectory: true) + try self.fileManager.createDirectory(at: url, withIntermediateDirectories: true, attributes: nil) + + let exists = self.fileManager.cachedFileExists(at: url) + + expect(exists) == false + try fileManager.removeItem(at: url) + } + + func testCachedFileExistsReturnsFalseForMissingFile() { + let url = self.testDirectory.appendingPathComponent("cached_file_missing.txt") + + let exists = self.fileManager.cachedFileExists(at: url) + + expect(exists) == false + } + // MARK: - loadFile Tests func testLoadFileReturnsCorrectData() throws { diff --git a/Tests/UnitTests/Mocks/MockLargeItemCache.swift b/Tests/UnitTests/Mocks/MockLargeItemCache.swift index 9aa431fe02..e4175e92bf 100644 --- a/Tests/UnitTests/Mocks/MockLargeItemCache.swift +++ b/Tests/UnitTests/Mocks/MockLargeItemCache.swift @@ -55,6 +55,13 @@ final class MockLargeItemCache: LargeItemCacheType { return storage[url] != nil } + func cachedFileExists(at url: URL) -> Bool { + lock.lock() + defer { lock.unlock() } + + return storage[url] != nil + } + func loadFile(at url: URL) throws -> Data { lock.lock() defer { lock.unlock() } diff --git a/Tests/UnitTests/Mocks/MockSimpleCache.swift b/Tests/UnitTests/Mocks/MockSimpleCache.swift index 3e310e4227..b5c5504638 100644 --- a/Tests/UnitTests/Mocks/MockSimpleCache.swift +++ b/Tests/UnitTests/Mocks/MockSimpleCache.swift @@ -84,6 +84,10 @@ class MockSimpleCache: LargeItemCacheType, @unchecked Sendable { } } + func cachedFileExists(at url: URL) -> Bool { + self.cachedContentExists(at: url) + } + func stubSaveData(at index: Int = 0, with result: Result) { lock.withLock { saveDataResponses.insert(result, at: index) diff --git a/Tests/UnitTests/Mocks/MockUnderlyingSynchronizedFileCache.swift b/Tests/UnitTests/Mocks/MockUnderlyingSynchronizedFileCache.swift index 5954c65628..12a253eb11 100644 --- a/Tests/UnitTests/Mocks/MockUnderlyingSynchronizedFileCache.swift +++ b/Tests/UnitTests/Mocks/MockUnderlyingSynchronizedFileCache.swift @@ -85,6 +85,10 @@ extension SynchronizedLargeItemCache { } } + func cachedFileExists(at url: URL) -> Bool { + self.cachedContentExists(at: url) + } + func loadFile(at url: URL) throws -> Data { try lock.withLock { loadFileInvocations.append(url) diff --git a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobFetcherTests.swift b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobFetcherTests.swift index a8b7bfa404..037b79bf79 100644 --- a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobFetcherTests.swift +++ b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobFetcherTests.swift @@ -337,17 +337,22 @@ final class RemoteConfigBlobFetcherTests: TestCase { expect(self.blobStore.invokedWriteCount) == 1 } - func testDifferentRefsRunConcurrentlyUpToWorkerLimit() async { + func testDifferentRefsRunConcurrentlyUpToWorkerLimit() async throws { let refs = (0..<6).map { Self.ref(for: "blob-\($0)".asData) } self.fetcher.prefetch(refs: refs) await self.downloader.waitForRequestCount(4) expect(self.downloader.activeRequestCount) == 4 + let firstBatch = Array(self.downloader.requestedRefs.prefix(4)) + expect(Set(firstBatch).isSubset(of: Set(refs))) == true + expect(Set(firstBatch)).to(haveCount(4)) - self.downloader.complete(ref: refs[0], with: .success("blob-0".asData)) + let activeRef = try XCTUnwrap(firstBatch.first) + let activeIndex = try XCTUnwrap(refs.firstIndex(of: activeRef)) + self.downloader.complete(ref: activeRef, with: .success("blob-\(activeIndex)".asData)) await self.downloader.waitForRequestCount(5) - expect(Array(self.downloader.requestedRefs.prefix(4))) == Array(refs.prefix(4)) + expect(self.downloader.activeRequestCount) == 4 } func testPrefetchSchedulesLowPriorityRefs() async { @@ -359,17 +364,19 @@ final class RemoteConfigBlobFetcherTests: TestCase { expect(Set(self.downloader.requestedRefs)) == Set(refs) } - func testOnDemandRequestRunsBeforeQueuedPrefetches() async { + func testOnDemandRequestRunsBeforeQueuedPrefetches() async throws { let prefetchRefs = (0..<5).map { Self.ref(for: "prefetch-\($0)".asData) } let onDemandPayload = "on demand".asData let onDemandRef = Self.ref(for: onDemandPayload) self.fetcher.prefetch(refs: prefetchRefs) await self.downloader.waitForRequestCount(4) + let activePrefetchRef = try XCTUnwrap(self.downloader.requestedRefs.prefix(4).first) + let activePrefetchIndex = try XCTUnwrap(prefetchRefs.firstIndex(of: activePrefetchRef)) let onDemand = Task { await self.fetcher.ensureDownloaded(ref: onDemandRef) } await self.waitForScheduledTaskToReachFetcher() - self.downloader.complete(ref: prefetchRefs[0], with: .success("prefetch-0".asData)) + self.downloader.complete(ref: activePrefetchRef, with: .success("prefetch-\(activePrefetchIndex)".asData)) await self.downloader.waitForRequestCount(5) expect(self.downloader.requestedRefs[4]) == onDemandRef @@ -379,17 +386,19 @@ final class RemoteConfigBlobFetcherTests: TestCase { expect(result) == true } - func testOnDemandRequestBoostsAndJoinsQueuedPrefetch() async { + func testOnDemandRequestBoostsAndJoinsQueuedPrefetch() async throws { let prefetchRefs = (0..<5).map { Self.ref(for: "prefetch-\($0)".asData) } let boostedPayload = "boosted".asData let boostedRef = Self.ref(for: boostedPayload) self.fetcher.prefetch(refs: prefetchRefs + [boostedRef]) await self.downloader.waitForRequestCount(4) + let activePrefetchRef = try XCTUnwrap(self.downloader.requestedRefs.prefix(4).first) + let activePrefetchIndex = try XCTUnwrap(prefetchRefs.firstIndex(of: activePrefetchRef)) let boosted = Task { await self.fetcher.ensureDownloaded(ref: boostedRef) } await self.waitForScheduledTaskToReachFetcher() - self.downloader.complete(ref: prefetchRefs[0], with: .success("prefetch-0".asData)) + self.downloader.complete(ref: activePrefetchRef, with: .success("prefetch-\(activePrefetchIndex)".asData)) await self.downloader.waitForRequestCount(5) expect(self.downloader.requestedRefs[4]) == boostedRef diff --git a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobStoreTests.swift b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobStoreTests.swift index 5cb72a1c74..44dfa90f57 100644 --- a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobStoreTests.swift +++ b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigBlobStoreTests.swift @@ -85,10 +85,10 @@ final class RemoteConfigBlobStoreTests: TestCase { func testCachedRefsRetriesDiskScanAfterTransientDirectoryListingFailure() { self.write(ref: Self.refA, data: Data([1])) - let fileManager = FailingFileManager() - let reopened = RemoteConfigBlobStore(fileManager: fileManager, directoryURL: self.directoryURL) + let cache = FailingCache() + let reopened = RemoteConfigBlobStore(cache: cache, directoryURL: self.directoryURL) - fileManager.failNextContentsOfDirectory = true + cache.failNextContentsOfDirectory = true expect(reopened.cachedRefs()).to(beEmpty()) expect(reopened.cachedRefs()) == [Self.refA] @@ -183,12 +183,12 @@ final class RemoteConfigBlobStoreTests: TestCase { } func testClearFailureDoesNotMarkExistingRefsAsMissing() { - let fileManager = FailingFileManager() - self.blobStore = RemoteConfigBlobStore(fileManager: fileManager, directoryURL: self.directoryURL) + let cache = FailingCache() + self.blobStore = RemoteConfigBlobStore(cache: cache, directoryURL: self.directoryURL) self.write(ref: Self.refA, data: Data([1])) expect(self.blobStore.contains(ref: Self.refA)) == true - fileManager.failNextRemoveItem = true + cache.failNextRemoveItem = true self.blobStore.clear() expect(self.blobStore.contains(ref: Self.refA)) == true @@ -215,8 +215,8 @@ final class RemoteConfigBlobStoreTests: TestCase { } func testRetainOnlyWaitsForInProgressWrite() { - let fileManager = BlockingFileManager() - self.blobStore = RemoteConfigBlobStore(fileManager: fileManager, directoryURL: self.directoryURL) + let cache = BlockingCache() + self.blobStore = RemoteConfigBlobStore(cache: cache, directoryURL: self.directoryURL) let writeFinished = DispatchSemaphore(value: 0) let retainStarted = DispatchSemaphore(value: 0) @@ -228,7 +228,7 @@ final class RemoteConfigBlobStoreTests: TestCase { writeFinished.signal() } - expect(fileManager.waitForDirectoryCreation()) == true + expect(cache.waitForDirectoryCreation()) == true DispatchQueue.global().async { retainStarted.signal() @@ -237,13 +237,13 @@ final class RemoteConfigBlobStoreTests: TestCase { } expect(retainStarted.wait(timeout: .now() + 1)) == .success - expect(fileManager.waitForContentsOfDirectory(timeout: .now() + 0.1)) == false + expect(cache.waitForContentsOfDirectory(timeout: .now() + 0.1)) == false - fileManager.unblockDirectoryCreation() + cache.unblockDirectoryCreation() expect(writeFinished.wait(timeout: .now() + 1)) == .success expect(retainFinished.wait(timeout: .now() + 1)) == .success - expect(fileManager.waitForContentsOfDirectory()) == true + expect(cache.waitForContentsOfDirectory()) == true } } @@ -262,7 +262,7 @@ private extension RemoteConfigBlobStoreTests { } -private final class FailingFileManager: FileManager { +private final class FailingCache: FileManager { var failNextContentsOfDirectory = false var failNextRemoveItem = false @@ -295,7 +295,7 @@ private final class FailingFileManager: FileManager { } -private final class BlockingFileManager: FileManager { +private final class BlockingCache: FileManager { private let enteredDirectoryCreation = DispatchSemaphore(value: 0) private let allowDirectoryCreation = DispatchSemaphore(value: 0) diff --git a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigIntegrationTests.swift b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigIntegrationTests.swift index 41b68bc84a..3a0adf22c0 100644 --- a/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigIntegrationTests.swift +++ b/Tests/UnitTests/Networking/RemoteConfig/RemoteConfigIntegrationTests.swift @@ -50,7 +50,7 @@ final class RemoteConfigIntegrationTests: TestCase { self.diskCache = RemoteConfigDiskCache(cache: synchronizedCache) self.blobStore = RemoteConfigBlobStore( - fileManager: .default, + cache: FileManager.default, directoryURL: cacheDirectoryURL.appendingPathComponent("blobs", isDirectory: true) ) self.sourceProvider = RemoteConfigSourceProvider(topicStore: self.diskCache)