Merge pull request #2148 from dreampiggy/refactor_disk_cache

Refactor storeImageDataToDisk and support writing options

SDWebImage/SDImageCache.h

@@ -150,15 +150,13 @@ typedef void(^SDWebImageCompletionWithPossibleErrorBlock)(NSError * _Nullable er
 /**
  * Synchronously store image NSData into disk cache at the given key.
  *
- * @warning This method is synchronous, make sure to call it from the ioQueue
- *
  * @param imageData  The image data to store
  * @param key        The unique image cache key, usually it's image absolute URL
- * @param errorPtr   NSError pointer. If error occurs then (*errorPtr) != nil.
+ * @param error      NSError pointer, for possible file I/O errors, See FoundationErrors.h
  */
 - (BOOL)storeImageDataToDisk:(nullable NSData *)imageData
                       forKey:(nullable NSString *)key
-                       error:(NSError * _Nullable * _Nullable)errorPtr;
+                       error:(NSError * _Nullable * _Nullable)error;
 
 
 #pragma mark - Query and Retrieve Ops

SDWebImage/SDImageCache.m

@@ -137,14 +137,6 @@ FOUNDATION_STATIC_INLINE NSUInteger SDCacheCostForImage(UIImage *image) {
     [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
-- (void)checkIfQueueIsIOQueue {
-    const char *currentQueueLabel = dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL);
-    const char *ioQueueLabel = dispatch_queue_get_label(self.ioQueue);
-    if (strcmp(currentQueueLabel, ioQueueLabel) != 0) {
-        NSLog(@"This method should be called from the ioQueue");
-    }
-}
-
 #pragma mark - Cache paths
 
 - (void)addReadOnlyCachePath:(nonnull NSString *)path {
@@ -233,7 +225,7 @@ FOUNDATION_STATIC_INLINE NSUInteger SDCacheCostForImage(UIImage *image) {
                     }
                     data = [[SDWebImageCodersManager sharedInstance] encodedDataWithImage:image format:format];
                 }
-                [self storeImageDataToDisk:data forKey:key error:&writeError];
+                [self safeStoreImageDataToDisk:data forKey:key error:&writeError];
             }
             
             if (completionBlock) {
@@ -251,15 +243,34 @@ FOUNDATION_STATIC_INLINE NSUInteger SDCacheCostForImage(UIImage *image) {
 
 - (BOOL)storeImageDataToDisk:(nullable NSData *)imageData
                       forKey:(nullable NSString *)key
-                       error:(NSError * _Nullable * _Nullable)errorPtr {
+                       error:(NSError * _Nullable __autoreleasing * _Nullable)error {
     if (!imageData || !key) {
         return NO;
     }
+    __autoreleasing NSError *fileError;
+    if (!error) {
+        error = &fileError;
+    }
     
-    [self checkIfQueueIsIOQueue];
+    __block BOOL success = YES;
+    void(^storeImageDataBlock)(void) =  ^{
+        success = [self safeStoreImageDataToDisk:imageData forKey:key error:error];
+    };
+    dispatch_sync(self.ioQueue, storeImageDataBlock);
     
+    return success;
+}
+
+- (BOOL)safeStoreImageDataToDisk:(nullable NSData *)imageData
+                          forKey:(nullable NSString *)key
+                           error:(NSError * _Nullable __autoreleasing * _Nonnull)error {
+    if (!imageData || !key) {
+        return NO;
+    }
     if (![_fileManager fileExistsAtPath:_diskCachePath]) {
-        [_fileManager createDirectoryAtPath:_diskCachePath withIntermediateDirectories:YES attributes:nil error:NULL];
+        if (![_fileManager createDirectoryAtPath:_diskCachePath withIntermediateDirectories:YES attributes:nil error:error]) {
+            return NO;
+        }
     }
     
     // get cache Path for image key
@@ -267,8 +278,9 @@ FOUNDATION_STATIC_INLINE NSUInteger SDCacheCostForImage(UIImage *image) {
     // transform to NSUrl
     NSURL *fileURL = [NSURL fileURLWithPath:cachePathForKey];
     
-    if (![_fileManager createFileAtPath:cachePathForKey contents:imageData attributes:nil] && errorPtr) {
+    // NSFileManager's `createFileAtPath:` is used just for old code compatibility and will not trigger any delegate methods, so it's useless for custom NSFileManager at all.
-        *errorPtr = [[NSError alloc] initWithDomain:NSPOSIXErrorDomain code:errno userInfo:nil];
+    // And also, NSFileManager's `createFileAtPath:` can only grab underlying POSIX errno, but NSData can grab errors defined in NSCocoaErrorDomain, which is better for user to check.
+    if (![imageData writeToFile:cachePathForKey options:self.config.diskCacheWritingOptions error:error]) {
         return NO;
     }
     

SDWebImage/SDImageCacheConfig.h

@@ -29,10 +29,16 @@
 
 /**
  * The reading options while reading cache from disk.
- * Defaults to 0. You can set this to mapped file to improve performance.
+ * Defaults to 0. You can set this to `NSDataReadingMappedIfSafe` to improve performance.
  */
 @property (assign, nonatomic) NSDataReadingOptions diskCacheReadingOptions;
 
+/**
+ * The writing options while writing cache to disk.
+ * Defaults to `NSDataWritingAtomic`. You can set this to `NSDataWritingWithoutOverwriting` to prevent overwriting an existing file.
+ */
+@property (assign, nonatomic) NSDataWritingOptions diskCacheWritingOptions;
+
 /**
  * The maximum length of time to keep an image in the cache, in seconds.
  */

SDWebImage/SDImageCacheConfig.m

@@ -18,6 +18,7 @@ static const NSInteger kDefaultCacheMaxCacheAge = 60 * 60 * 24 * 7; // 1 week
         _shouldDisableiCloud = YES;
         _shouldCacheImagesInMemory = YES;
         _diskCacheReadingOptions = 0;
+        _diskCacheWritingOptions = NSDataWritingAtomic;
         _maxCacheAge = kDefaultCacheMaxCacheAge;
         _maxCacheSize = 0;
     }

SDWebImage/SDWebImageImageIOCoder.m

@@ -540,8 +540,6 @@ static const CGFloat kDestSeemOverlap = 2.0f;   // the numbers of pixels to over
                 result = [SDWebImageCoderHelper imageOrientationFromEXIFOrientation:exifOrientation];
             } // else - if it's not set it remains at up
             CFRelease((CFTypeRef) properties);
-        } else {
-            //NSLog(@"NO PROPERTIES, FAIL");
         }
         CFRelease(imageSource);
     }

Tests/Tests/SDImageCacheTests.m

@@ -309,23 +309,28 @@ NSString *kImageTestKey = @"TestImageKey.jpg";
 
 - (void)test41StoreImageDataToDiskWithError {
     NSData *imageData = [NSData dataWithContentsOfFile:[self testImagePath]];
+    NSError *targetError = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteNoPermissionError userInfo:nil];
     NSError *error = nil;
+    SDMockFileManager *fileManager = [[SDMockFileManager alloc] init];
+    fileManager.mockSelectors = @{NSStringFromSelector(@selector(createDirectoryAtPath:withIntermediateDirectories:attributes:error:)) : targetError};
     SDImageCache *cache = [[SDImageCache alloc] initWithNamespace:@"test"
                                                diskCacheDirectory:@"/"
-                                                      fileManager:[[SDMockFileManager alloc] initWithError:EACCES]];
+                                                      fileManager:fileManager];
     [cache storeImageDataToDisk:imageData
                          forKey:kImageTestKey
                           error:&error];
     
-    XCTAssertEqual(error.code, EACCES);
+    XCTAssertEqual(error.code, NSFileWriteNoPermissionError);
 }
 
 - (void)test42StoreImageDataToDiskWithoutError {
     NSData *imageData = [NSData dataWithContentsOfFile:[self testImagePath]];
     NSError *error = nil;
+    SDMockFileManager *fileManager = [[SDMockFileManager alloc] init];
+    fileManager.mockSelectors = @{NSStringFromSelector(@selector(createDirectoryAtPath:withIntermediateDirectories:attributes:error:)) : [NSNull null]};
     SDImageCache *cache = [[SDImageCache alloc] initWithNamespace:@"test"
                                                diskCacheDirectory:@"/"
-                                                      fileManager:[[SDMockFileManager alloc] initWithError:0]];
+                                                      fileManager:fileManager];
     [cache storeImageDataToDisk:imageData
                          forKey:kImageTestKey
                           error:&error];

Tests/Tests/SDMockFileManager.h

@@ -8,8 +8,9 @@
 
 #import <Foundation/Foundation.h>
 
+// This is a mock class to provide custom error for methods
 @interface SDMockFileManager : NSFileManager
 
-- (id)initWithError:(int)errorNumber;
+@property (nonatomic, copy, nullable) NSDictionary<NSString *, NSError *> *mockSelectors; // used to specify mocked selectors which will return NO with specify error instead of normal process. If you specify a NSNull, will use nil instead.
 
 @end

Tests/Tests/SDMockFileManager.m

@@ -10,24 +10,25 @@
 
 @interface SDMockFileManager ()
 
-@property (nonatomic, assign) int errorNumber;
-
 @end
 
 @implementation SDMockFileManager
 
-- (id)initWithError:(int)errorNumber {
+- (BOOL)createDirectoryAtPath:(NSString *)path withIntermediateDirectories:(BOOL)createIntermediates attributes:(NSDictionary<NSFileAttributeKey,id> *)attributes error:(NSError * _Nullable __autoreleasing *)error {
-    self = [super init];
+    NSError *mockError = [self.mockSelectors objectForKey:NSStringFromSelector(_cmd)];
-    if (self) {
+    if ([mockError isEqual:[NSNull null]]) {
-        _errorNumber = errorNumber;
+        if (error) {
+            *error = nil;
         }
-    
+        return NO;
-    return self;
+    } else if (mockError) {
+        if (error) {
+            *error = mockError;
+        }
+        return NO;
+    } else {
+        return [super createDirectoryAtPath:path withIntermediateDirectories:createIntermediates attributes:attributes error:error];
     }
-
-- (BOOL)createFileAtPath:(NSString *)path contents:(NSData *)data attributes:(NSDictionary<NSString *,id> *)attr {
-    errno = self.errorNumber;
-    return (self.errorNumber == 0);
 }
 
 @end