Merge pull request #3788 from dreampiggy/bugfix/cache_query_type_optimization

Fix the issue that previous optimization for special case (multiple same URL in prefetcher list) breaks the `queryCacheType` option sematic

.github/workflows/CI.yml

@@ -45,12 +45,12 @@ jobs:
       DEVELOPER_DIR: /Applications/Xcode_16.0.app
       WORKSPACE_NAME: SDWebImage.xcworkspace
       CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
-      iosDestination: platform=iOS Simulator,name=iPhone 16 Pro
+      iosDestination: generic/platform=iOS Simulator
       macOSDestination: platform=macOS,arch=x86_64
       macCatalystDestination: platform=macOS,arch=x86_64,variant=Mac Catalyst
-      tvOSDestination: platform=tvOS Simulator,name=Apple TV 4K (3rd generation)
-      watchOSDestination: platform=watchOS Simulator,name=Apple Watch Ultra 2 (49mm)
-      visionOSDestination: platform=visionOS Simulator,name=Apple Vision Pro
+      tvOSDestination: generic/platform=tvOS Simulator
+      watchOSDestination: generic/platform=watchOS Simulator
+      visionOSDestination: generic/platform=visionOS Simulator
     steps:
       - name: Checkout
         uses: actions/checkout@v3
@@ -106,7 +106,7 @@ jobs:
         platform: [iOS, macOS, tvOS, visionOS]
         include:
           - platform: iOS
-            destination: platform=iOS Simulator,name=iPhone 16 Pro
+            destination: platform=iOS Simulator,name=iPhone 16 Pro Max
             scheme: iOS
             flag: ios
           - platform: macOS

SDWebImage/Core/SDImageCache.m

@@ -619,7 +619,8 @@ static NSString * _defaultDiskCacheDirectory;
     
     // First check the in-memory cache...
     UIImage *image;
-    if (queryCacheType != SDImageCacheTypeDisk) {
+    BOOL shouldQueryDiskOnly = (queryCacheType == SDImageCacheTypeDisk);
+    if (!shouldQueryDiskOnly) {
         image = [self imageFromMemoryCacheForKey:key];
     }
     
@@ -683,14 +684,19 @@ static NSString * _defaultDiskCacheDirectory;
             // the image is from in-memory cache, but need image data
             diskImage = image;
         } else if (diskData) {
+            // the image memory cache miss, need image data and image
             BOOL shouldCacheToMemory = YES;
             if (context[SDWebImageContextStoreCacheType]) {
                 SDImageCacheType cacheType = [context[SDWebImageContextStoreCacheType] integerValue];
                 shouldCacheToMemory = (cacheType == SDImageCacheTypeAll || cacheType == SDImageCacheTypeMemory);
             }
-            // Special case: If user query image in list for the same URL, to avoid decode and write **same** image object into disk cache multiple times, we query and check memory cache here again.
-            if (shouldCacheToMemory && self.config.shouldCacheImagesInMemory) {
-                diskImage = [self.memoryCache objectForKey:key];
+            // Special case: If user query image in list for the same URL, to avoid decode and write **same** image object into disk cache multiple times, we query and check memory cache here again. See: #3523
+            // This because disk operation can be async, previous sync check of `memory cache miss`, does not gurantee current check of `memory cache miss`
+            if (!shouldQueryDiskSync) {
+                // First check the in-memory cache...
+                if (!shouldQueryDiskOnly) {
+                    diskImage = [self imageFromMemoryCacheForKey:key];
+                }
             }
             // decode image data only if in-memory cache missed
             if (!diskImage) {

Tests/Tests/SDAnimatedImageTest.m

@@ -313,14 +313,17 @@ static BOOL _isCalled;
 - (void)test22AnimatedImageViewCategory {
     XCTestExpectation *expectation = [self expectationWithDescription:@"test SDAnimatedImageView view category"];
     SDAnimatedImageView *imageView = [SDAnimatedImageView new];
-    NSURL *testURL = [NSURL URLWithString:kTestGIFURL];
-    [imageView sd_setImageWithURL:testURL completed:^(UIImage * _Nullable image, NSError * _Nullable error, SDImageCacheType cacheType, NSURL * _Nullable imageURL) {
+    NSURL *testURL = [NSURL URLWithString:@"https://media.giphy.com/media/3oeji6siihbdrxxi40/giphy.gif"];
+    [SDImageCache.sharedImageCache removeImageFromMemoryForKey:testURL.absoluteString];
+    [SDImageCache.sharedImageCache removeImageFromDiskForKey:testURL.absoluteString];
+    // I don't know why, but `fromLoaderOnly` is need for iOS Unit Test on GitHub Action
+    [imageView sd_setImageWithURL:testURL placeholderImage:nil options:SDWebImageFromLoaderOnly completed:^(UIImage * _Nullable image, NSError * _Nullable error, SDImageCacheType cacheType, NSURL * _Nullable imageURL) {
         expect(error).to.beNil();
         expect(image).notTo.beNil();
         expect([image isKindOfClass:[SDAnimatedImage class]]).beTruthy();
         [expectation fulfill];
     }];
-    [self waitForExpectationsWithCommonTimeout];
+    [self waitForExpectationsWithTimeout:kAsyncTestTimeout * 2 handler:nil];
 }
 
 - (void)test23AnimatedImageViewCategoryProgressive {

Tests/Tests/SDTestCase.m

@@ -14,7 +14,7 @@ const int64_t kMinDelayNanosecond = NSEC_PER_MSEC * 100; // 0.1s
 NSString *const kTestJPEGURL = @"https://placehold.co/50x50.jpg";
 NSString *const kTestProgressiveJPEGURL = @"https://raw.githubusercontent.com/ibireme/YYImage/master/Demo/YYImageDemo/mew_progressive.jpg";
 NSString *const kTestPNGURL = @"https://placehold.co/50x50.png";
-NSString *const kTestGIFURL = @"https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif";
+NSString *const kTestGIFURL = @"https://upload.wikimedia.org/wikipedia/commons/3/31/Eokxd.gif";
 NSString *const kTestAPNGPURL = @"https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png";
 
 @implementation SDTestCase

Tests/Tests/SDWebImageManagerTests.m

@@ -377,16 +377,22 @@
     NSURL *url = [NSURL URLWithString:@"https://placehold.co/101x101.png"];
     NSString *key = [SDWebImageManager.sharedManager cacheKeyForURL:url];
     NSData *testImageData = [NSData dataWithContentsOfFile:[self testJPEGPath]];
+    UIImage *testImage = [UIImage sd_imageWithData:testImageData];
     [SDImageCache.sharedImageCache storeImageDataToDisk:testImageData forKey:key];
     
     // Query memory first
     [SDWebImageManager.sharedManager loadImageWithURL:url options:SDWebImageFromCacheOnly context:@{SDWebImageContextQueryCacheType : @(SDImageCacheTypeMemory)} progress:nil completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, SDImageCacheType cacheType, BOOL finished, NSURL * _Nullable imageURL) {
         expect(image).beNil();
         expect(cacheType).equal(SDImageCacheTypeNone);
+        // Store the image to memory
+        [SDImageCache.sharedImageCache storeImageToMemory:testImage forKey:key];
         // Query disk secondly
         [SDWebImageManager.sharedManager loadImageWithURL:url options:SDWebImageFromCacheOnly context:@{SDWebImageContextQueryCacheType : @(SDImageCacheTypeDisk)} progress:nil completed:^(UIImage * _Nullable image2, NSData * _Nullable data2, NSError * _Nullable error2, SDImageCacheType cacheType2, BOOL finished2, NSURL * _Nullable imageURL2) {
             expect(image2).notTo.beNil();
             expect(cacheType2).equal(SDImageCacheTypeDisk);
+            // We should ensure that this disk image is not the same one in-memory
+            expect(image2 != testImage).beTruthy();
+            [SDImageCache.sharedImageCache removeImageFromMemoryForKey:key];
             [SDImageCache.sharedImageCache removeImageFromDiskForKey:key];
             [expectation fulfill];
         }];