Use a lock instead of barrier queue to avoid dispatch_sync blocking the main queue on race condition
Showing
1 changed file
with
62 additions
and
37 deletions
@@ -9,6 +9,10 @@ | @@ -9,6 +9,10 @@ | ||
9 | #import "SDWebImageDownloader.h" | 9 | #import "SDWebImageDownloader.h" |
10 | #import "SDWebImageDownloaderOperation.h" | 10 | #import "SDWebImageDownloaderOperation.h" |
11 | 11 | ||
12 | +#define LOCK(...) dispatch_semaphore_wait(self->_lock, DISPATCH_TIME_FOREVER); \ | ||
13 | +__VA_ARGS__; \ | ||
14 | +dispatch_semaphore_signal(self->_lock); | ||
15 | + | ||
12 | @interface SDWebImageDownloadToken () | 16 | @interface SDWebImageDownloadToken () |
13 | 17 | ||
14 | @property (nonatomic, weak, nullable) NSOperation<SDWebImageDownloaderOperationInterface> *downloadOperation; | 18 | @property (nonatomic, weak, nullable) NSOperation<SDWebImageDownloaderOperationInterface> *downloadOperation; |
@@ -36,8 +40,7 @@ | @@ -36,8 +40,7 @@ | ||
36 | @property (assign, nonatomic, nullable) Class operationClass; | 40 | @property (assign, nonatomic, nullable) Class operationClass; |
37 | @property (strong, nonatomic, nonnull) NSMutableDictionary<NSURL *, SDWebImageDownloaderOperation *> *URLOperations; | 41 | @property (strong, nonatomic, nonnull) NSMutableDictionary<NSURL *, SDWebImageDownloaderOperation *> *URLOperations; |
38 | @property (strong, nonatomic, nullable) SDHTTPHeadersMutableDictionary *HTTPHeaders; | 42 | @property (strong, nonatomic, nullable) SDHTTPHeadersMutableDictionary *HTTPHeaders; |
39 | -// This queue is used to serialize the handling of the network responses of all the download operation in a single queue | ||
40 | -@property (strong, nonatomic, nullable) dispatch_queue_t barrierQueue; | 43 | +@property (strong, nonatomic, nonnull) dispatch_semaphore_t lock; // a lock to keep the access to `URLOperations` thread-safe |
41 | 44 | ||
42 | // The session in which data tasks will run | 45 | // The session in which data tasks will run |
43 | @property (strong, nonatomic) NSURLSession *session; | 46 | @property (strong, nonatomic) NSURLSession *session; |
@@ -96,7 +99,7 @@ | @@ -96,7 +99,7 @@ | ||
96 | #else | 99 | #else |
97 | _HTTPHeaders = [@{@"Accept": @"image/*;q=0.8"} mutableCopy]; | 100 | _HTTPHeaders = [@{@"Accept": @"image/*;q=0.8"} mutableCopy]; |
98 | #endif | 101 | #endif |
99 | - _barrierQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderBarrierQueue", DISPATCH_QUEUE_CONCURRENT); | 102 | + _lock = dispatch_semaphore_create(1); |
100 | _downloadTimeout = 15.0; | 103 | _downloadTimeout = 15.0; |
101 | 104 | ||
102 | [self createNewSessionWithConfiguration:sessionConfiguration]; | 105 | [self createNewSessionWithConfiguration:sessionConfiguration]; |
@@ -231,13 +234,15 @@ | @@ -231,13 +234,15 @@ | ||
231 | } | 234 | } |
232 | 235 | ||
233 | - (void)cancel:(nullable SDWebImageDownloadToken *)token { | 236 | - (void)cancel:(nullable SDWebImageDownloadToken *)token { |
234 | - dispatch_barrier_async(self.barrierQueue, ^{ | ||
235 | - SDWebImageDownloaderOperation *operation = self.URLOperations[token.url]; | ||
236 | - BOOL canceled = [operation cancel:token.downloadOperationCancelToken]; | ||
237 | - if (canceled) { | ||
238 | - [self.URLOperations removeObjectForKey:token.url]; | ||
239 | - } | ||
240 | - }); | 237 | + NSURL *url = token.url; |
238 | + if (!url) { | ||
239 | + return; | ||
240 | + } | ||
241 | + SDWebImageDownloaderOperation *operation = [self operationForURL:url]; | ||
242 | + BOOL canceled = [operation cancel:token.downloadOperationCancelToken]; | ||
243 | + if (canceled) { | ||
244 | + [self removeOperationForURL:url]; | ||
245 | + } | ||
241 | } | 246 | } |
242 | 247 | ||
243 | - (nullable SDWebImageDownloadToken *)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock | 248 | - (nullable SDWebImageDownloadToken *)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock |
@@ -251,33 +256,24 @@ | @@ -251,33 +256,24 @@ | ||
251 | } | 256 | } |
252 | return nil; | 257 | return nil; |
253 | } | 258 | } |
254 | - | ||
255 | - __block SDWebImageDownloadToken *token = nil; | ||
256 | - | ||
257 | - dispatch_barrier_sync(self.barrierQueue, ^{ | ||
258 | - SDWebImageDownloaderOperation *operation = self.URLOperations[url]; | ||
259 | - if (!operation) { | ||
260 | - operation = createCallback(); | ||
261 | - self.URLOperations[url] = operation; | ||
262 | - | ||
263 | - __weak SDWebImageDownloaderOperation *woperation = operation; | ||
264 | - operation.completionBlock = ^{ | ||
265 | - dispatch_barrier_sync(self.barrierQueue, ^{ | ||
266 | - SDWebImageDownloaderOperation *soperation = woperation; | ||
267 | - if (!soperation) return; | ||
268 | - if (self.URLOperations[url] == soperation) { | ||
269 | - [self.URLOperations removeObjectForKey:url]; | ||
270 | - }; | ||
271 | - }); | ||
272 | - }; | ||
273 | - } | ||
274 | - id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock]; | ||
275 | - | ||
276 | - token = [SDWebImageDownloadToken new]; | ||
277 | - token.downloadOperation = operation; | ||
278 | - token.url = url; | ||
279 | - token.downloadOperationCancelToken = downloadOperationCancelToken; | ||
280 | - }); | 259 | + |
260 | + SDWebImageDownloaderOperation *operation = [self operationForURL:url]; | ||
261 | + if (!operation) { | ||
262 | + operation = createCallback(); | ||
263 | + [self setOperation:operation forURL:url]; | ||
264 | + | ||
265 | + __weak typeof(self) wself = self; | ||
266 | + operation.completionBlock = ^{ | ||
267 | + __strong typeof(wself) sself = wself; | ||
268 | + [sself removeOperationForURL:url]; | ||
269 | + }; | ||
270 | + } | ||
271 | + id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock]; | ||
272 | + | ||
273 | + SDWebImageDownloadToken *token = [SDWebImageDownloadToken new]; | ||
274 | + token.downloadOperation = operation; | ||
275 | + token.url = url; | ||
276 | + token.downloadOperationCancelToken = downloadOperationCancelToken; | ||
281 | 277 | ||
282 | return token; | 278 | return token; |
283 | } | 279 | } |
@@ -292,6 +288,35 @@ | @@ -292,6 +288,35 @@ | ||
292 | 288 | ||
293 | #pragma mark Helper methods | 289 | #pragma mark Helper methods |
294 | 290 | ||
291 | +- (SDWebImageDownloaderOperation *)operationForURL:(NSURL *)url { | ||
292 | + if (!url) { | ||
293 | + return nil; | ||
294 | + } | ||
295 | + SDWebImageDownloaderOperation *operation; | ||
296 | + LOCK({ | ||
297 | + operation = [self.URLOperations objectForKey:url]; | ||
298 | + }); | ||
299 | + return operation; | ||
300 | +} | ||
301 | + | ||
302 | +- (void)setOperation:(SDWebImageDownloaderOperation *)operation forURL:(NSURL *)url { | ||
303 | + if (!operation || !url) { | ||
304 | + return; | ||
305 | + } | ||
306 | + LOCK({ | ||
307 | + [self.URLOperations setObject:operation forKey:url]; | ||
308 | + }); | ||
309 | +} | ||
310 | + | ||
311 | +- (void)removeOperationForURL:(NSURL *)url { | ||
312 | + if (!url) { | ||
313 | + return; | ||
314 | + } | ||
315 | + LOCK({ | ||
316 | + [self.URLOperations removeObjectForKey:url]; | ||
317 | + }); | ||
318 | +} | ||
319 | + | ||
295 | - (SDWebImageDownloaderOperation *)operationWithTask:(NSURLSessionTask *)task { | 320 | - (SDWebImageDownloaderOperation *)operationWithTask:(NSURLSessionTask *)task { |
296 | SDWebImageDownloaderOperation *returnOperation = nil; | 321 | SDWebImageDownloaderOperation *returnOperation = nil; |
297 | for (SDWebImageDownloaderOperation *operation in self.downloadQueue.operations) { | 322 | for (SDWebImageDownloaderOperation *operation in self.downloadQueue.operations) { |
-
Please register or login to post a comment