ibireme / YYCache

High performance cache framework for iOS.
MIT License
2.38k stars 511 forks source link

[YYKVStorage] initWithPath should return error when directory already exists #10

Closed skyline75489 closed 8 years ago

skyline75489 commented 8 years ago

The file creation part from YYKVStorage.m:

    NSError *error = nil;
    if (![[NSFileManager defaultManager] createDirectoryAtPath:path
                                   withIntermediateDirectories:YES
                                                    attributes:nil
                                                         error:&error] ||
        ![[NSFileManager defaultManager] createDirectoryAtPath:[path stringByAppendingPathComponent:kDataDirectoryName]
                                   withIntermediateDirectories:YES
                                                    attributes:nil
                                                         error:&error] ||
        ![[NSFileManager defaultManager] createDirectoryAtPath:[path stringByAppendingPathComponent:kTrashDirectoryName]
                                   withIntermediateDirectories:YES
                                                    attributes:nil
                                                         error:&error]) {
        NSLog(@"YYKVStorage init error:%@", error);
        return nil;
    }

This will not result in an error if directory already exists.

ibireme commented 8 years ago

When something is stored in the file directory and you may re-open it at the next time, so it should not return error.

skyline75489 commented 8 years ago

Yet if we use [[YYCache alloc] initWithName:@"com.me.cache"]; twice with the same name, the database will malfunction because of sqlite lock:

2015-12-02 12:47:49.954 Keyboard[2373:38138] -[YYKVStorage _dbUpdateAccessTimeWithKey:] line:230 sqlite update error (5): database is locked
2015-12-02 12:47:52.458 Keyboard[2373:38079] -[YYKVStorage _dbSaveWithKey:value:fileName:extendedData:] line:216 sqlite insert error (5): database is locked
ibireme commented 8 years ago

Yes, it will make the storage unstable, see these comments:

https://github.com/ibireme/YYCache/blob/master/YYCache/YYDiskCache.h#L148 https://github.com/ibireme/YYCache/blob/master/YYCache/YYKVStorage.h#L99 https://github.com/ibireme/YYCache/blob/master/YYCache/YYCache.h#L58 https://github.com/ibireme/YYCache/blob/master/YYCache/YYCache.h#L69

skyline75489 commented 8 years ago

I didn't really notice those comments until now...I think it would better to error out or something like that.

ibireme commented 8 years ago

Yeah..maybe I should give an error.. I will consider it later.

kharmabum commented 8 years ago

I was also experiencing errors related to a locked database. Can we reuse names/paths in order to reuse a YYCache that was created on a previous run?

At the moment I am doing:

    lazy var cache: YYCache = {

        let cache = YYCache(path: NSURL(string: NSTemporaryDirectory())!.URLByAppendingPathComponent(NSUUID().UUIDString).URLString)
        cache.memoryCache.countLimit = 100
        cache.diskCache.countLimit = 100
        return cache
    }()

What's the recommend way of creating/reusing and old YYCache or is this not possible? Could you clarify what you mean by "When something is stored in the file directory and you may re-open it at the next time, so it should not return error." This seems to conflict with Multiple instances with the same path/name will make the storage unstable.

ibireme commented 8 years ago

Currently, I recommend to create only one YYCache/YYDiskCache instance for a specified path in your app (singleton).

I will do some improvements to avoid this issues.

ibireme commented 8 years ago

Updated:

If the disk cache instance for the specified path already exists in memory, the initialize method will return it directly, instead of creating a new instance.