Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dead lock #507

Closed
cherpake opened this issue Sep 22, 2013 · 10 comments
Closed

Dead lock #507

cherpake opened this issue Sep 22, 2013 · 10 comments
Labels
Milestone

Comments

@cherpake
Copy link

Dead lock when using - (BOOL)diskImageExistsForURL:(NSURL *)url; to check if image exists on the disc.

As it's calling:

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

    return exists;
    }

and another thread is waiting on:

        dispatch_main_sync_safe(^
        {
            doneBlock(diskImage, SDImageCacheTypeDisk);
        });

in this method: - (NSOperation *)queryDiskCacheForKey:(NSString *)key done:(void (^)(UIImage *image, SDImageCacheType cacheType))doneBlock;

@cherpake
Copy link
Author

Some additional info, i tried to fix this issue by:

  1. removing the dispatch_sync in the diskImageExistsWithKey:(NSString *)key method - which resolved it, but scrolling between the images wasn't fluid (UI).
  2. changing the macro dispatch_main_sync_safe to by async - made UI much more fluid and responsive

Now i saw that this change was introduced recently and probably for a good reason, but doing dispatch_sync on the main thread IMHO doesn't seem as a good idea, as it might be other wise busy, why not use dispatch_async here?

@cherpake
Copy link
Author

One last note: checking this commit 3a6d948 i see that you turned a lot of async calls to sync calls, just saying.

@dperetti
Copy link

FYI, I'm experiencing the exact same issue at the moment :-)

@dreipol
Copy link

dreipol commented Jan 31, 2014

Got the same issue.
Please apply cherpakes patch, it fixes it.

@bpoplauschi
Copy link
Member

Related to #509

@nicksnyder
Copy link

I can reliably reproduce this problem using SDWebImage 3.7.2, Xcode 6.2, in the iPhone 6 iOS 8.2 simulator.

See this project:
https://github.com/nicksnyder/iOS-UIWebView-vs-UITableView

@mythodeia
Copy link
Contributor

@nicksnyder you produce the deadlock by setting the maxConcurrentDownloads to 1024. This sets the maxConcurrentOperationCount of the downloadQueue in SDWebImageDownloader to 1024 so NSOperationQueue will try to execute at the same time 1024 operations. This will surely hurt the system.
I tried it with as less as 70 in my system and still it deadlocks. 60 is the magic number for me to make it work

@nicksnyder
Copy link

I didn't mean to imply that 1024 was the smallest number needed to deadlock, but it does seem to vary based on the hardware being run on (which is why I put a silly high number in the bug report).

32 concurrent operations performs better than 3 (the default) in my project. I was manually searching for the sweet spot which is when I reproduced the deadlock. The deadlock is indeed still a bug with SDWebImage, right?

@mythodeia
Copy link
Contributor

i think that the deadlock in SDWebImage occurs because of race conditions happening while requests are cancelled or finished. NSOperationQueue does not dequeue an operation until its finished state changes to true. So there might be some issues there.

@bpoplauschi
Copy link
Member

Should be fixed by the latest 4.x branch changes (Redone the stuff from #781 by 062e50a f7e8246 c77adf4 265ace4 0c47bc3. Same stuff: replaced all sync calls to the completion with async, cleaned up the code a bit).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants