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

ImageTask.setPriority(_:) sometimes crashes #226

Closed
keynsianets opened this issue Apr 24, 2019 · 13 comments

Comments

Projects
None yet
3 participants
@keynsianets
Copy link

commented Apr 24, 2019

Since 21.04.2019 we get a lot of crashes, related to Nuke in app. Before we get no crashes and i guess new version had broke something critical. Now 3% our users have crashes in app. I attach screenshots from fabric.
Снимок экрана 2019-04-24 в 09 58 44
Снимок экрана 2019-04-24 в 09 58 17
Снимок экрана 2019-04-24 в 09 57 50
Снимок экрана 2019-04-24 в 09 57 17
Снимок экрана 2019-04-24 в 09 56 42
Снимок экрана 2019-04-24 в 09 55 53
Снимок экрана 2019-04-24 в 09 59 20

@kean

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

Thanks for the report, @keynsianets. I will investigate. It might potentially be related to poor unfair_lock usage which was fixed in 7.6.2

@kean kean added the bug label Apr 24, 2019

@michaelnisi

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Looks a lot like #224, which seems to be resolved, at least my manual testing didn’t trigger the issue with the latest version. Thank you, Alex, for your super quick reaction—as always. 🙌

@keynsianets

This comment has been minimized.

Copy link
Author

commented Apr 25, 2019

Thank you for response. We will test it in a new build and tell you about changes. But it will take time.

@keynsianets

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

Hello. In our new build with Nuke SDK version 7.6.2 problem not fixed. I attach new screenshots from fabric. We use prefetching images with disk cache only and loading images in tableview.
Prefetching code:
let preheater = ImagePreheater(destination: .diskCache) preheater.startPreheating(with: urls)
Loading code:
Nuke.loadImage(with: url, options: options, into: self, progress: nil, completion: { response, error in self.loadingIndicator.stopAnimating() })?.setPriority(.veryHigh)
Снимок экрана 2019-04-30 в 09 40 00
Снимок экрана 2019-04-30 в 09 40 32
Снимок экрана 2019-04-30 в 09 40 55
Снимок экрана 2019-04-30 в 09 41 29
Снимок экрана 2019-04-30 в 09 42 06
Снимок экрана 2019-04-30 в 09 42 27
Снимок экрана 2019-04-30 в 09 42 44
Снимок экрана 2019-04-30 в 09 42 58

@kean

This comment has been minimized.

Copy link
Owner

commented Apr 30, 2019

I would suggest reverting to the previous version for now.

@keynsianets

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

I would suggest reverting to the previous version for now.

Thank you for so fast reaction and great work on this framework. Yes, i think in next build we will use previous version of Nuke and waiting for fixes of this issue in future.

@kean

This comment has been minimized.

Copy link
Owner

commented Apr 30, 2019

It doesn't seem that the issue is widespread. Nuke has all sorts of unit tests, thread safety tests – none reported the problem. If you could share some more information about the crash, that would be helpful. It might be related to the way it is used, could you please shared a snippet of how you use it?

@keynsianets

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

We have aws backend for images. From API we get urls for our tableview. General, it's a feed and it's nearly infinity. On this tableview we have 2 methods with loading images. Prefetch:
let preheater = ImagePreheater(destination: .diskCache) func tableView(_ tableView: UITableView, prefetchRowsAt indexPaths: [IndexPath]) { //processing urls preheater.startPreheating(with: urls) }
And loading:
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell { // set other data for cell and set image for imageView in it let options = ImageLoadingOptions( placeholder: UIImage(), failureImage: UIImage(), contentModes: .init( success: .scaleAspectFit, failure: .center, placeholder: .center ) ) Nuke.loadImage(with: url, options: options, into: self, progress: nil, completion: { response, error in self.loadingIndicator.stopAnimating() })?.setPriority( .veryHigh) }
I tested scrolling on this feed 1000-2000 images and it didn't crash. But we have 3% user with crashes now. If you need some other information - ask, i will try to provide it as fast, as i can.

@michaelnisi

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

kean added a commit that referenced this issue Apr 30, 2019

@kean

This comment has been minimized.

Copy link
Owner

commented Apr 30, 2019

I found one obvious threading defect in ImageTask.setPriority(_:) method and fixed it. I'm not yet absolutely sure that this is the defect but it certainly seems like it is. Here's a test case that simulates the situation.

What is concerning is that this doesn't seem like a regression introduced in 7.6, it seems that ImageTask.setPriority(_:) method always had this defect. @keynsianets when did you start using ImageTask.setPriority(_:) method?

As a workaround I suggest not to use ImageTask.setPriority(_:) method:

var request = ImageRequest(url: url)
request.priority = .veryHigh

Nuke.loadImage(with: request, into: ...)

P.S. there is quite a lot of complexity in ImagePipeline which was introduced in Nuke 7, I would really like to address it in one of the upcoming releases.

@keynsianets

This comment has been minimized.

Copy link
Author

commented May 1, 2019

Maybe sePriority(_:) is the issue. We start using this method and prefetch at the same time with update to new Nuke version. In next build we will try don't use ImagePreheater(destination: .diskCache) in prefetch and replace sePriority(_:) with

var request = ImageRequest(url: url)
request.priority = .veryHigh

Thank you a lot for your work.

@kean

This comment has been minimized.

Copy link
Owner

commented May 1, 2019

Thanks, I will investigate a bit further and release a patch soon.

Initially I though ImagePreheater(destination: .diskCache) was the problem – it's also a relatively rarely used feature – but it wasn't. I think you can safely continue using it. The only problem is ImageTask.setPriority(_:).

@kean kean changed the title A lot of crashes in new version ImageTask.setPriority threading issue May 1, 2019

@kean kean changed the title ImageTask.setPriority threading issue ImageTask.setPriority sometimes crashes May 1, 2019

@kean kean changed the title ImageTask.setPriority sometimes crashes ImageTask.setPriority(_:) sometimes crashes May 1, 2019

@kean kean added this to the 7.6.3 milestone May 1, 2019

@kean

This comment has been minimized.

Copy link
Owner

commented May 1, 2019

@kean kean closed this May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.