Fix race condition in image download cancellation

There are many race conditions around cancelling
SDWebImageDownloaderOperation instances from other threads. For example,
imageData may be set to nil and deallocated just as it is being appended
to, or the threading can interleave in such a way that messages are sent
to a deallocated connection. These were discovered using SDWebImage for
a Google Maps-style tiled mapping application where there is a lot of
download and cancellation if users pan rapidly.

This fix tracks the worker thread that the NSURLConnection instance runs on and
performs cancellation on the worker thread. The cancel and start methods need
to be synchronized to handle the case where cancellation happens before
start is called; since no thread has been assigned yet, cancellation is
performed on the calling thread.

Because cancellation is now scheduled on the same run loop as
NSURLConnectionDelegate callbacks, there is an added window of time that
a download can finish prior to cancellation. This means it's possible to
cancel an operation yet still get a successful completion callback. This
was always possible because of race conditions, but it is more
pronounced and predictable now. An application that relies on
a cancelled operation never calling its completion block (e.g. recycling
image views in a scrolling table view) must adopt some other mechanism
(e.g. a version number) to avoid running completion code for a stale operation.
This commit is contained in:
Erik Charlebois 2013-12-09 17:08:56 -05:00
parent 3380e56a7d
commit 3f20a101c5
1 changed files with 45 additions and 21 deletions

View File

@ -22,6 +22,7 @@
@property (assign, nonatomic) long long expectedSize;
@property (strong, nonatomic) NSMutableData *imageData;
@property (strong, nonatomic) NSURLConnection *connection;
@property (strong, atomic) NSThread *thread;
#if TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_4_0
@property (assign, nonatomic) UIBackgroundTaskIdentifier backgroundTaskId;
@ -54,34 +55,36 @@
- (void)start
{
if (self.isCancelled)
{
self.finished = YES;
[self reset];
return;
}
@synchronized(self) {
if (self.isCancelled)
{
self.finished = YES;
[self reset];
return;
}
#if TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_4_0
if ([self shouldContinueWhenAppEntersBackground])
{
__weak __typeof__(self) wself = self;
self.backgroundTaskId = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^
if ([self shouldContinueWhenAppEntersBackground])
{
__strong __typeof(wself)sself = wself;
if (sself)
__weak __typeof__(self) wself = self;
self.backgroundTaskId = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^
{
[sself cancel];
__strong __typeof(wself)sself = wself;
[[UIApplication sharedApplication] endBackgroundTask:sself.backgroundTaskId];
sself.backgroundTaskId = UIBackgroundTaskInvalid;
}
}];
}
if (sself)
{
[sself cancel];
[[UIApplication sharedApplication] endBackgroundTask:sself.backgroundTaskId];
sself.backgroundTaskId = UIBackgroundTaskInvalid;
}
}];
}
#endif
self.executing = YES;
self.connection = [NSURLConnection.alloc initWithRequest:self.request delegate:self startImmediately:NO];
self.executing = YES;
self.connection = [NSURLConnection.alloc initWithRequest:self.request delegate:self startImmediately:NO];
}
[self.connection start];
@ -121,6 +124,26 @@
}
- (void)cancel
{
@synchronized(self) {
if (self.thread)
{
[self performSelector:@selector(cancelInternalAndStop) onThread:self.thread withObject:nil waitUntilDone:NO];
}
else
{
[self cancelInternal];
}
}
}
- (void)cancelInternalAndStop
{
[self cancelInternal];
CFRunLoopStop(CFRunLoopGetCurrent());
}
- (void)cancelInternal
{
if (self.isFinished) return;
[super cancel];
@ -154,6 +177,7 @@
self.progressBlock = nil;
self.connection = nil;
self.imageData = nil;
self.thread = nil;
}
- (void)setFinished:(BOOL)finished