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

KingfisherManager retrieveImage completionHandler not called on specified queue #1066

Closed
fgulan opened this issue Dec 11, 2018 · 6 comments · Fixed by #1067
Closed

KingfisherManager retrieveImage completionHandler not called on specified queue #1066

fgulan opened this issue Dec 11, 2018 · 6 comments · Fixed by #1067

Comments

@fgulan
Copy link

fgulan commented Dec 11, 2018

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

What

After upgrading from 4.x to 5.x this snippet of code:

KingfisherManager.shared.retrieveImage(with: url, options: [.callbackQueue(.mainAsync)]) { result in
    // invoked on background thread
}

doesn't respect .callbackQueue option and callback is executed on some background thread. Callback is called (background thread) from:

KingfisherManager.retrieveImageFromCache(source:options:completionHandler:) at KingfisherManager.swift:324

Currently I don't have time for further debug, but I suspect on method:

ImageCache.retrieveImage(forKey:options:callbackQueue:completionHandler:) at ImageCache.swift:440

since there is a default parameter for callbackQueue which is .untouch and options are not parsed at all.

Reproduce

Currently I've managed to reproduce this issue only when image is loaded form disk cache. For downloading image and in memory cache it seems that it works fine.

@onevcat
Copy link
Owner

onevcat commented Dec 11, 2018

Yes, you are 100% right on this.

The default value of .untouch in ImageCache gives a chance to "overwrite" the default .mainCurrentOrAsync in options (since there should be no need to dispatch to the main queue). It is a performance trick but I forgot to use the callback queue at an upper level. I will give it a fix now.

@onevcat
Copy link
Owner

onevcat commented Dec 11, 2018

Currently I've managed to reproduce this issue only when image is loaded form disk cache. For downloading image and in memory cache it seems that it works fine.

I guess loading from memory cache would also cause this problem...

@aksswami
Copy link

aksswami commented Dec 12, 2018

I am also facing this issue after migrating to 5.0. As a workaround for now, I have switched to main thread in completion handler.

Btw @onevcat thanks for the v5, It improved a lot of things and resolved couple of pressing issues.

@rocxteady
Copy link

I am using version 5.3.1. I face the same problem. Any progress on this? My code looks like this:

    let options: KingfisherOptionsInfo = [.callbackQueue(.mainAsync)]
        ImageCache.default.retrieveImage(forKey: key, options: options, completionHandler: { (result) in
            switch result {
            case .success(let value):
                
            case .failure(let error):
               
            }
        })

Thanks.

@onevcat
Copy link
Owner

onevcat commented Apr 9, 2019

@rocxteady Hi, this method is a bit different for internal performance reason. You need to specify the callback queue explicitly, otherwise, it uses .untouch to avoid unnecessary dispatch.

ImageCache.default.retrieveImage(
    forKey: key, options: options, callbackQueue: .mainAsync, completionHandler: { /* */  }
)

@rocxteady
Copy link

@onevcat thank you for the information. But it's a bit misleasing. I thought we could set a callback queue. Maybe you should change it we can't set it this way. Therefore there will not be any misleading. Thank you again.

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

Successfully merging a pull request may close this issue.

4 participants