SDWebImage#625 More fixes against deadlocks. dispatch_sync on a seria…

…l queue is an anti pattern, as it can lead to deadlocks. I replaced all the dispatch_main_sync_safe with dispatch_main_async_safe, based on the fact that the completion blocks are not returning anything, so we don't need to wait for them to finish. The biggest change (architectural) is that the completion block will no longer be executed inside the operation. But that is not an issue, as NSOperation does the same (completion block gets called after operation isFinished).
bpoplauschi · Jun 25, 2014 · 03be256 · 03be256
1 parent 46d23ad
commit 03be256
Show file tree

Hide file tree

Showing 7 changed files with 27 additions and 33 deletions.
diff --git a/SDWebImage/MKAnnotationView+WebCache.m b/SDWebImage/MKAnnotationView+WebCache.m
@@ -48,7 +48,7 @@ - (void)sd_setImageWithURL:(NSURL *)url placeholderImage:(UIImage *)placeholder
         __weak MKAnnotationView *wself = self;
         id <SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:url options:options progress:nil completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
             if (!wself) return;
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 __strong MKAnnotationView *sself = wself;
                 if (!sself) return;
                 if (image) {

diff --git a/SDWebImage/SDWebImageCompat.h b/SDWebImage/SDWebImageCompat.h
@@ -53,13 +53,6 @@
 
 extern UIImage *SDScaledImageForKey(NSString *key, UIImage *image);
 
-#define dispatch_main_sync_safe(block)\
-    if ([NSThread isMainThread]) {\
-        block();\
-    } else {\
-        dispatch_sync(dispatch_get_main_queue(), block);\
-    }
-
 #define dispatch_main_async_safe(block)\
     if ([NSThread isMainThread]) {\
         block();\

diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m
@@ -291,7 +291,9 @@ - (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data {
                 UIImage *scaledImage = [self scaledImageForKey:key image:image];
                 image = [UIImage decodedImageWithImage:scaledImage];
                 CGImageRelease(partialImageRef);
-                dispatch_main_sync_safe(^{
+
+                // this is only executed in case we have a valid partial image
+                dispatch_main_async_safe(^{
                     if (self.completedBlock) {
                         self.completedBlock(image, nil, nil, NO);
                     }

diff --git a/SDWebImage/SDWebImageManager.m b/SDWebImage/SDWebImageManager.m
@@ -134,11 +134,11 @@ - (void)diskImageExistsForURL:(NSURL *)url
     }
 
     if (!url || (!(options & SDWebImageRetryFailed) && isFailedUrl)) {
-        dispatch_main_sync_safe(^{
+        dispatch_main_async_safe(^{
             NSError *error = [NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorFileDoesNotExist userInfo:nil];
             completedBlock(nil, error, SDImageCacheTypeNone, YES, url);
         });
-        return operation;
+        return nil; // no need to return the operation here, as we have an error
     }
 
     @synchronized (self.runningOperations) {
@@ -157,7 +157,7 @@ - (void)diskImageExistsForURL:(NSURL *)url
 
         if ((!image || options & SDWebImageRefreshCached) && (![self.delegate respondsToSelector:@selector(imageManager:shouldDownloadImageForURL:)] || [self.delegate imageManager:self shouldDownloadImageForURL:url])) {
             if (image && options & SDWebImageRefreshCached) {
-                dispatch_main_sync_safe(^{
+                dispatch_main_async_safe(^{
                     // If image was found in the cache bug SDWebImageRefreshCached is provided, notify about the cached image
                     // AND try to re-download it in order to let a chance to NSURLCache to refresh it from server.
                     completedBlock(image, nil, cacheType, YES, url);
@@ -181,12 +181,12 @@ - (void)diskImageExistsForURL:(NSURL *)url
             }
             id <SDWebImageOperation> subOperation = [self.imageDownloader downloadImageWithURL:url options:downloaderOptions progress:progressBlock completed:^(UIImage *downloadedImage, NSData *data, NSError *error, BOOL finished) {
                 if (weakOperation.isCancelled) {
-                    dispatch_main_sync_safe(^{
+                    dispatch_main_async_safe(^{
                         completedBlock(nil, nil, SDImageCacheTypeNone, finished, url);
                     });
                 }
                 else if (error) {
-                    dispatch_main_sync_safe(^{
+                    dispatch_main_async_safe(^{
                         completedBlock(nil, error, SDImageCacheTypeNone, finished, url);
                     });
 
@@ -212,7 +212,7 @@ - (void)diskImageExistsForURL:(NSURL *)url
                                 [self.imageCache storeImage:transformedImage recalculateFromImage:imageWasTransformed imageData:data forKey:key toDisk:cacheOnDisk];
                             }
 
-                            dispatch_main_sync_safe(^{
+                            dispatch_main_async_safe(^{
                                 completedBlock(transformedImage, nil, SDImageCacheTypeNone, finished, url);
                             });
                         });
@@ -222,7 +222,7 @@ - (void)diskImageExistsForURL:(NSURL *)url
                             [self.imageCache storeImage:downloadedImage recalculateFromImage:NO imageData:data forKey:key toDisk:cacheOnDisk];
                         }
 
-                        dispatch_main_sync_safe(^{
+                        dispatch_main_async_safe(^{
                             completedBlock(downloadedImage, nil, SDImageCacheTypeNone, finished, url);
                         });
                     }
@@ -243,7 +243,7 @@ - (void)diskImageExistsForURL:(NSURL *)url
             };
         }
         else if (image) {
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 completedBlock(image, nil, cacheType, YES, url);
             });
             @synchronized (self.runningOperations) {
@@ -252,7 +252,7 @@ - (void)diskImageExistsForURL:(NSURL *)url
         }
         else {
             // Image not in cache and download disallowed by delegate
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 completedBlock(nil, nil, SDImageCacheTypeNone, YES, url);
             });
             @synchronized (self.runningOperations) {

diff --git a/SDWebImage/UIButton+WebCache.m b/SDWebImage/UIButton+WebCache.m
@@ -71,7 +71,7 @@ - (void)sd_setImageWithURL:(NSURL *)url forState:(UIControlState)state placehold
     __weak UIButton *wself = self;
     id <SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:url options:options progress:nil completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
         if (!wself) return;
-        dispatch_main_sync_safe(^{
+        dispatch_main_async_safe(^{
             __strong UIButton *sself = wself;
             if (!sself) return;
             if (image) {
@@ -114,7 +114,7 @@ - (void)sd_setBackgroundImageWithURL:(NSURL *)url forState:(UIControlState)state
         __weak UIButton *wself = self;
         id <SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:url options:options progress:nil completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
             if (!wself) return;
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 __strong UIButton *sself = wself;
                 if (!sself) return;
                 if (image) {

diff --git a/SDWebImage/UIImageView+HighlightedWebCache.m b/SDWebImage/UIImageView+HighlightedWebCache.m
@@ -36,17 +36,16 @@ - (void)sd_setHighlightedImageWithURL:(NSURL *)url options:(SDWebImageOptions)op
         __weak UIImageView      *wself    = self;
         id<SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:url options:options progress:progressBlock completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
             if (!wself) return;
-            dispatch_main_sync_safe (^
-                                     {
-                                         if (!wself) return;
-                                         if (image) {
-                                             wself.highlightedImage = image;
-                                             [wself setNeedsLayout];
-                                         }
-                                         if (completedBlock && finished) {
-                                             completedBlock(image, error, cacheType, url);
-                                         }
-                                     });
+            dispatch_main_async_safe (^{
+                if (!wself) return;
+                if (image) {
+                    wself.highlightedImage = image;
+                    [wself setNeedsLayout];
+                }
+                if (completedBlock && finished) {
+                    completedBlock(image, error, cacheType, url);
+                }
+            });
         }];
         [self sd_setImageLoadOperation:operation forKey:UIImageViewHighlightedWebCacheOperationKey];
     } else {

diff --git a/SDWebImage/UIImageView+WebCache.m b/SDWebImage/UIImageView+WebCache.m
@@ -50,7 +50,7 @@ - (void)sd_setImageWithURL:(NSURL *)url placeholderImage:(UIImage *)placeholder
         __weak UIImageView *wself = self;
         id <SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:url options:options progress:progressBlock completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
             if (!wself) return;
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 if (!wself) return;
                 if (image) {
                     wself.image = image;
@@ -97,7 +97,7 @@ - (void)sd_setAnimationImagesWithURLs:(NSArray *)arrayOfURLs {
     for (NSURL *logoImageURL in arrayOfURLs) {
         id <SDWebImageOperation> operation = [SDWebImageManager.sharedManager downloadImageWithURL:logoImageURL options:0 progress:nil completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, BOOL finished, NSURL *imageURL) {
             if (!wself) return;
-            dispatch_main_sync_safe(^{
+            dispatch_main_async_safe(^{
                 __strong UIImageView *sself = wself;
                 [sself stopAnimating];
                 if (sself && image) {