Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cache crash when no disk space left #1628

Merged
merged 3 commits into from
Feb 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 53 additions & 18 deletions Sources/Cache/DiskStorage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,36 +50,44 @@ public enum DiskStorage {
var maybeCached : Set<String>?
let maybeCachedCheckingQueue = DispatchQueue(label: "com.onevcat.Kingfisher.maybeCachedCheckingQueue")

// `false` if the storage initialized with an error. This prevents unexpected forcibly crash when creating
// storage in the default cache.
private var storageReady: Bool = true

/// Creates a disk storage with the given `DiskStorage.Config`.
///
/// - Parameter config: The config used for this disk storage.
/// - Throws: An error if the folder for storage cannot be got or created.
public init(config: Config) throws {
public convenience init(config: Config) throws {
self.init(noThrowConfig: config, creatingDirectory: false)
try prepareDirectory()
}

self.config = config
// If `creatingDirectory` is `false`, the directory preparation will be skipped.
// We need to call `prepareDirectory` manually after this returns.
init(noThrowConfig config: Config, creatingDirectory: Bool) {
var config = config

let url: URL
if let directory = config.directory {
url = directory
} else {
url = try config.fileManager.url(
for: .cachesDirectory,
in: .userDomainMask,
appropriateFor: nil,
create: true)
}
let creation = Creation(config)
self.directoryURL = creation.directoryURL

let cacheName = "com.onevcat.Kingfisher.ImageCache.\(config.name)"
directoryURL = config.cachePathBlock(url, cacheName)
// Break any possible retain cycle set by outside.
config.cachePathBlock = nil
self.config = config

metaChangingQueue = DispatchQueue(label: cacheName)
metaChangingQueue = DispatchQueue(label: creation.cacheName)
setupCacheChecking()

try prepareDirectory()
if creatingDirectory {
try? prepareDirectory()
}
}

private func setupCacheChecking() {
maybeCachedCheckingQueue.async {
do {
self.maybeCached = Set()
try config.fileManager.contentsOfDirectory(atPath: self.directoryURL.path).forEach { fileName in
try self.config.fileManager.contentsOfDirectory(atPath: self.directoryURL.path).forEach { fileName in
self.maybeCached?.insert(fileName)
}
} catch {
Expand All @@ -91,7 +99,7 @@ public enum DiskStorage {
}

// Creates the storage folder.
func prepareDirectory() throws {
private func prepareDirectory() throws {
let fileManager = config.fileManager
let path = directoryURL.path

Expand All @@ -103,6 +111,7 @@ public enum DiskStorage {
withIntermediateDirectories: true,
attributes: nil)
} catch {
self.storageReady = false
throw KingfisherError.cacheError(reason: .cannotCreateDirectory(path: path, error: error))
}
}
Expand All @@ -112,6 +121,10 @@ public enum DiskStorage {
forKey key: String,
expiration: StorageExpiration? = nil) throws
{
guard storageReady else {
throw KingfisherError.cacheError(reason: .diskStorageIsNotReady(cacheURL: directoryURL))
}

let expiration = expiration ?? config.expiration
// The expiration indicates that already expired, no need to store.
guard !expiration.isExpired else { return }
Expand Down Expand Up @@ -167,6 +180,10 @@ public enum DiskStorage {
actuallyLoad: Bool,
extendingExpiration: ExpirationExtending) throws -> T?
{
guard storageReady else {
throw KingfisherError.cacheError(reason: .diskStorageIsNotReady(cacheURL: directoryURL))
}

let fileManager = config.fileManager
let fileURL = cacheFileURL(forKey: key)
let filePath = fileURL.path
Expand Down Expand Up @@ -491,3 +508,21 @@ extension DiskStorage {
}
}

extension DiskStorage {
struct Creation {
let directoryURL: URL
let cacheName: String

init(_ config: Config) {
let url: URL
if let directory = config.directory {
url = directory
} else {
url = config.fileManager.urls(for: .cachesDirectory, in: .userDomainMask)[0]
}

cacheName = "com.onevcat.Kingfisher.ImageCache.\(config.name)"
directoryURL = config.cachePathBlock(url, cacheName)
}
}
}
48 changes: 42 additions & 6 deletions Sources/Cache/ImageCache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ open class ImageCache {
/// for any of your customize cache.
public static let `default` = ImageCache(name: "default")


// MARK: Public Properties
/// The `MemoryStorage.Backend` object used in this cache. This storage holds loaded images in memory with a
/// reasonable expire duration and a maximum memory usage. To modify the configuration of a storage, just set
Expand Down Expand Up @@ -213,7 +214,7 @@ open class ImageCache {
/// You should not use the same `name` for different caches, otherwise, the disk storage would
/// be conflicting to each other. The `name` should not be an empty string.
public convenience init(name: String) {
try! self.init(name: name, cacheDirectoryURL: nil, diskCachePathClosure: nil)
self.init(noThrowName: name, cacheDirectoryURL: nil, diskCachePathClosure: nil)
}

/// Creates an `ImageCache` with a given `name`, cache directory `path`
Expand All @@ -233,17 +234,55 @@ open class ImageCache {
public convenience init(
name: String,
cacheDirectoryURL: URL?,
diskCachePathClosure: DiskCachePathClosure? = nil) throws
diskCachePathClosure: DiskCachePathClosure? = nil
) throws
{
if name.isEmpty {
fatalError("[Kingfisher] You should specify a name for the cache. A cache with empty name is not permitted.")
}

let memoryStorage = ImageCache.createMemoryStorage()

let config = ImageCache.createConfig(
name: name, cacheDirectoryURL: cacheDirectoryURL, diskCachePathClosure: diskCachePathClosure
)
let diskStorage = try DiskStorage.Backend<Data>(config: config)
self.init(memoryStorage: memoryStorage, diskStorage: diskStorage)
}

convenience init(
noThrowName name: String,
cacheDirectoryURL: URL?,
diskCachePathClosure: DiskCachePathClosure?
)
{
if name.isEmpty {
fatalError("[Kingfisher] You should specify a name for the cache. A cache with empty name is not permitted.")
}

let memoryStorage = ImageCache.createMemoryStorage()

let config = ImageCache.createConfig(
name: name, cacheDirectoryURL: cacheDirectoryURL, diskCachePathClosure: diskCachePathClosure
)
let diskStorage = DiskStorage.Backend<Data>(noThrowConfig: config, creatingDirectory: true)
self.init(memoryStorage: memoryStorage, diskStorage: diskStorage)
}

private static func createMemoryStorage() -> MemoryStorage.Backend<KFCrossPlatformImage> {
let totalMemory = ProcessInfo.processInfo.physicalMemory
let costLimit = totalMemory / 4
let memoryStorage = MemoryStorage.Backend<KFCrossPlatformImage>(config:
.init(totalCostLimit: (costLimit > Int.max) ? Int.max : Int(costLimit)))
return memoryStorage
}

private static func createConfig(
name: String,
cacheDirectoryURL: URL?,
diskCachePathClosure: DiskCachePathClosure? = nil
) -> DiskStorage.Config
{
var diskConfig = DiskStorage.Config(
name: name,
sizeLimit: 0,
Expand All @@ -252,10 +291,7 @@ open class ImageCache {
if let closure = diskCachePathClosure {
diskConfig.cachePathBlock = closure
}
let diskStorage = try DiskStorage.Backend<Data>(config: diskConfig)
diskConfig.cachePathBlock = nil

self.init(memoryStorage: memoryStorage, diskStorage: diskStorage)
return diskConfig
}

deinit {
Expand Down
12 changes: 12 additions & 0 deletions Sources/General/KingfisherError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ public enum KingfisherError: Error {
/// - error: The underlying error originally thrown by Foundation when setting the `attributes` to the disk
/// file at `filePath`.
case cannotSetCacheFileAttribute(filePath: String, attributes: [FileAttributeKey : Any], error: Error)

/// The disk storage of cache is not ready. Code 3011.
///
/// This is usually due to extremely lack of space on disk storage, and
/// Kingfisher failed even when creating the cache folder. The disk storage will be in unusable state. Normally,
/// ask user to free some spaces and restart the app to make the disk storage work again.
/// - cacheURL: The intended URL which should be the storage folder.
case diskStorageIsNotReady(cacheURL: URL)
}


Expand Down Expand Up @@ -382,6 +390,9 @@ extension KingfisherError.CacheErrorReason {
case .cannotSetCacheFileAttribute(let filePath, let attributes, let error):
return "Cannot set file attribute for the cache file at path: \(filePath), attributes: \(attributes)." +
"Underlying foundation error: \(error)."
case .diskStorageIsNotReady(let cacheURL):
return "The disk storage is not ready to use yet at URL: '\(cacheURL)'. " +
"This is usually caused by extremely lack of disk space. Ask users to free up some space and restart the app."
}
}

Expand All @@ -397,6 +408,7 @@ extension KingfisherError.CacheErrorReason {
case .cannotSerializeImage: return 3008
case .cannotCreateCacheFile: return 3009
case .cannotSetCacheFileAttribute: return 3010
case .diskStorageIsNotReady: return 3011
}
}
}
Expand Down