Race in SDWebImageManager; occasional wrong image. #699
Conversation
Do not execute completion block if we've been cancelled. dispatch_main_sync_safe() does not guarantee that no other code runs until our block is executed, so the operation may have been cancelled in-between. If we call the completion block after cancellation, there's a race between this load and the subsequent image load in a table view cell, for instance.
| @@ -136,13 +136,12 @@ - (BOOL)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(^{ | |||
| completedBlock(nil, nil, SDImageCacheTypeNone, finished); | |||
| }); | |||
There was a problem hiding this comment.
You leave an empty condition, I guess it's an error isn't it?
|
Nope. It is intentional. I want to not go on and do the other things in the if/else construct if the operation is cancelled. Perhaps should be rewritten to better reflect this, but I wanted to keep intent of the commit/PR as clear as possible (i.e. change flow as little as possible) |
|
Could you add a comment in the body of the condition then? |
|
Done! |
|
@salling could you rebase this on top of master so it can be merged? |
…ne operation is cancelled, the completion block must not be called, otherwise it might race with a newer completion for the same object
|
Replaced by #781. Closing |
…on is cancelled, the completion block must not be called, otherwise it might race with a newer completion for the same object Conflicts: SDWebImage/SDWebImageManager.m
|
@salling Can you elaborate more on what this fixes and how does it? We are experiencing some completion blocks that are never executed when using concurrently with the SDWebImagePrefetcher, and I wonder if this change is related. Wouldn't you want to notify the caller via the completion block even on a cancelled operation? The operation could be cancelled by a completely different call elsewhere, couldn't it? |
…ne operation is cancelled, the completion block must not be called, otherwise it might race with a newer completion for the same object Conflicts: SDWebImage/SDWebImageManager.m
Do not execute completion block if we've been cancelled. dispatch_main_sync_safe() does not guarantee that no other code runs until our block is executed, so the operation may have been cancelled in-between. If we call the completion block after cancellation, there's a race between this load and the subsequent image load in a table view cell, for instance.