SDImageCache diskImageExistsWithKey may leads dead lock

[GooooodGuy]

call this method in main thread :

• (BOOL)diskImageExistsWithKey:(NSString *)key
  {
  __block BOOL exists = NO;
  dispatch_sync(_ioQueue, ^
  {
  exists = [_fileManager fileExistsAtPath:[self defaultCachePathForKey:key]];
  });

  return exists;
  }
  while SDImageCache ioQueue execute dispatch_main_sync_safe in method
  -(NSOperation *)queryDiskCacheForKey:(NSString *)key done:(void (^)(UIImage *image, SDImageCacheType cacheType))doneBlock ,line 308.

So,Can we implement the method as follow :

• (BOOL)diskImageExistsWithKey:(NSString *)key
  {
  NSFileManager *fileManager = [[NSFileManager alloc]init];
  BOOL exists = [fileManager fileExistsAtPath:[self defaultCachePathForKey:key]];

  return exists;
  }

Thanks !

[donholly]

The only way that could happen is if you call diskImageExistsWithKey: from the ioQueue (dispatching synchronous work to the same serial queue does cause a deadlock).

The assumption was made that this would be unlikely called from that queue, however it does need to be on that queue since NSFileManager is not threadsafe and was created on the ioQueue. We could instead prevent the possibility of a deadlock by doing something similar to dispatch_main_sync_safe, but instead checks the label on the provided queue and assures it is not running on that queue before dispatching to it synchronously.

Seem reasonable?

[rogermoffatt]

Hmmm ... I'm taking an interest because I'm getting deadlocks frequently at present.

https://developer.apple.com/library/ios/documentation/Cocoa/Reference/Foundation/Classes/NSFileManager_Class/Reference/Reference.html

"The methods of the shared NSFileManager object can be called from multiple threads safely. However, if you use a delegate to receive notifications about the status of move, copy, remove, and link operations, you should create a unique instance of the file manager object, assign your delegate to that object, and use that file manager to initiate your operations."

I can fix my problems by modifying SDWebImage to ignore the synchronous ioQueue.

[donholly]

Interesting! Good catch. Seems that if you use the defaultManager instance of NSFileManager we should be fine without the synchronous queue. That may be a slight performance boost, too.

[joshuaseltzer]

I'm seeing the same problem in my application. I fixed the problem by using the defaultManager for NSFileManager in the diskImageExistsWithKey method and removing the synchronous block (as mentioned above).

[leafduo]

Same problem here, and issue #507 is the same thing. Could we fix it?

[gossnj]

This is an ongoing issue for my project as well.

[iOSBrett]

I have also changed my version to use default manager to fix a 14 second deadlock I was experiencing:

• (BOOL)diskImageExistsWithKey:(NSString *)key
  {
  return [[NSFileManager defaultManager] fileExistsAtPath:[self defaultCachePathForKey:key]];
  }

Have not encountered any issues with initial testing and my App uses SDWebImage heavily.

[matej]

Yes, I think there really is a problem here and it's actually broader that this specific example.

It's pretty easy to see how we can get to a deadlock here. If diskImageExistsWithKey: gets called while queryDiskCacheForKey:done: is doing work on the ioQueue, the main thread will block on the dispatch_sync call inside diskImageExistsWithKey:. When queryDiskCacheForKey:done: in turn reaches the dispatch_main_sync_safe call, it has to wait for the main thread, which is blocked by the dispatch_sync call. The ioQueue waits for the main_queue and vice versa.

This can also happen in other places where dispatch_main_sync_safe is used to invoke callbacks (calculateSizeWithCompletionBlock:) and perhaps also in other places throughout the library - related to other queues.

In general I think it's pretty risky to invoke callbacks from a queue to main queue synchronously, if there are places where the main queue can enter the particular queue in a synchronous manner. To play it safe, think callbacks should rather be always dispatched asynchronously as a general rule (meaning 3a6d948 and any similar modifications should perhaps be reverted).

[donholly]

Seems reasonable. Quick fix would be to just use the [NSFileManager
defaultManager] since it is threadsafe.

Simple pull-request should do the trick :)

On Mon, Mar 24, 2014 at 5:14 AM, Matej Bukovinski
notifications@github.comwrote:

Yes, I think there really is a problem here and it's actually broader that
this specific example.

It's pretty easy to how to get to a deadlock here. If
diskImageExistsWithKey: gets called while queryDiskCacheForKey:done: is
doing work on the ioQueue, the main thread will block on the dispatch_synccall inside
diskImageExistsWithKey:. When queryDiskCacheForKey:done: in turn reaches
the dispatch_main_sync_safe call, it has to wait for the main thread,
which is blocked by the dispatch_sync call. The ioQueue waits for the
main_queue and vice versa.

This can also happen in other places where dispatch_main_sync_safe is used
to invoke callbacks (calculateSizeWithCompletionBlock:) and perhaps also
in other places throughout the library - related to other queues.

In general I think it's pretty risky to invoke callbacks from a queue to
main queue synchronously, if there are places where the main queue can
enter the particular queue in a synchronous manner. To play it safe, think
callbacks should rather be always dispatched asynchronously as a general
rule.

Reply to this email directly or view it on GitHubhttps://github.com//issues/625#issuecomment-38436894
.

[matej]

I came up with a couple of fixes for SDImageCache, that hopefully improve things a bit.

I guess that using the shared file manager would be ok here, however in my opinion, always using the serial ioQueue does have some advantages here (for instance making sure that operations that require multiple NSFileManager actions complete as one unit of work before the next cache task is performed). Also when using dispatch_async, we obviously also free up the main thread (not really applicable when discussing diskImageExistsWithKey: though).

I also think that using dispatch_async for the callbacks is more appropriate here, since we're always on the ioQueue and dispatch_main_sync_safe would not really save us much time here.

[bpoplauschi]

Fixed by commit 6e4fbaf.