Avoid premature completion of prefetcher if request fails

[robertmryan]

This addresses a number of issues, namely:

• This resolves bug in prefetcher that could result in premature notification of completion if one of the requests failed, as discussed in issue SDWebImagePrefetcher wrong completion block condition if skipped any images  #715.
• This attempts to make documentation in headers a little less ambiguous regarding the intent of the finishedCount parameter, making it clear that it includes both successful and unsuccessful requests.
• Unrelated, also fix race condition in SDWebimageDownloader.

[robertmryan]

@rs

You might want to rebuild the appledoc-based class framework documentation to reflect the header documentation changes.

[bpoplauschi]

@rs Please review as well, especially the else if (self.finishedCount == self.requestedCount) condition. Looks legit to me, but I'm still not experienced with this code.

[rs]

The else if (self.finishedCount == self.requestedCount) looks ok, but I'm wondering about the weak ref changed to strong ref. Could you please elaborate on this @robertmryan?

[robertmryan]

Technically, in this case, it doesn't matter, because the weakSelf/strongSelf pattern isn't necessary at all in this particular scenario. But if you're going to do what this code is doing, affectionately known as the "weakSelf/strongSelf dance", you might as well avoid race conditions that often plague multithreaded code.

First, to illustrate the recommended pattern, look at Transitioning to ARC Release Notes, there is a section called "Use Lifetime Qualifiers to Avoid Strong Reference Cycles", in which there is an example that says:

For non-trivial cycles, however, you should use:

MyViewController *myController = [[MyViewController alloc] init…];

// ...
MyViewController * __weak weakMyController = myController;
myController.completionHandler =  ^(NSInteger result) {
    MyViewController *strongMyController = weakMyController;
    if (strongMyController) {
        // ...
        [strongMyController dismissViewControllerAnimated:YES completion:nil];
        // ...
    }
    else {
        // Probably nothing...
    }
};

Note, you assign the weak object reference to a strong variable first, and then check to see if that strong variable is non-nil.

If you do it the other way around (check to see if the weak variable is non-nil, and only then set the strong variable), there is a race condition, namely that it's theoretically possible for the weak variable to become nil between the time you check to see that it's non-nil, and the time you assign the strong variable.

Usually, when you do this weakSelf/strongSelf dance, you're doing it because you want to ensure that strongSelf is not nil. So my change achieves that. It's not critical in this particular situation, but I just break out in hives anytime I see code that introduces theoretical race conditions. No offense was intended by my edit.

[bpoplauschi]

@robertmryan thanks for the detailed explanation. You are 100% correct.