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

Image does not load after being cancelled and instantly reloaded #55

Closed
MartinChan1 opened this issue Nov 26, 2015 · 8 comments
Closed
Assignees
Milestone

Comments

@MartinChan1
Copy link

imageView.af_setImageWithURL(url)
imageView.af_cancelImageRequest()
imageView.af_setImageWithURL(url)

The real world case we were having was:

  1. in a collectionview, a cell calls af_setImageWithURL.
  2. The collectionview is reloaded, then in the prepareForReuse() the request is cancelled.
  3. If the same cell makes the same image url request the image does not load

Testing on iOS 8 & iOS 9 this fails on simulator and device

@cnoon cnoon self-assigned this Dec 10, 2015
@cnoon
Copy link
Member

cnoon commented Dec 10, 2015

Hi @MartinChan1, thanks for opening this issue. If there is truly an issue with AlamofireImage here...we certainly want to fix it up!

With that said, I've dug heavily through the request and cancellation logic and I can't track down how you would be hitting this edge case. Cancellation should not cause any issues like this. I've created a new test case in b5f5b5e to verify everything works as expected.

I'm going to close this issue out for now. If you could submit a sample project that demonstrates the issue, that would be tremendously helpful. If you provide any information that leads us to believe there really is an issue with AlamofireImage directly, then I'll be more than happy to re-open and investigate further.

Cheers. 🍻

@MartinChan1
Copy link
Author

Hi @cnoon I checked the test and the first image url does not match the second url. It only fails if the 2 urls are equal. The first is https://httpbin.org/image/jpeg, the second is https://httpbin.org/image/png

@cnoon
Copy link
Member

cnoon commented Dec 10, 2015

Oh man apologies @MartinChan1. That's my bad. I'll re-open this issue and continue to investigate this evening. Thanks for catching my mistake!

@cnoon cnoon reopened this Dec 10, 2015
@cnoon
Copy link
Member

cnoon commented Dec 11, 2015

Okay @MartinChan1, I managed to get to the bottom problem and fixed the issue in 3dc1390.

This ended up being a very difficult issue to figure out how to solve in a backwards compatible manner. The core problem ended up being a race condition in our cancellation logic. We had always assumed you would cancel a download and would restart a different one. After digging into this scenario a bit more, I now realize that was actually a really poor assumption. Our download image logic was relying on the response request to match the current receipt request, otherwise we would early out. That always works if you're switching the images, but it NEVER works if you are reusing the same URL before the main thread has a chance to cycle.

Unfortunately, in most prepareForReuse scenarios, you will cancel the current request and start a new one all before the main thread has a chance to cycle. Because of this, it's pretty easy to hit this scenario.

If we weren't concerned about backwards compatibility, we would pass the RequestReceipt to the completion closure of the downloadImage API on the ImageDownloader. Since that's not ideal, I've added a card to our Trello backlog for the AFI 3.0 release. In the meantime, I've added the concept of a currentDownloadID that lives in the private global scope of the extension. That allows us to pass the unique identifier into the completion closure to allow us to check that it hasn't changed out from under us. This additional check allows us to handle this scenario as one would expect.

I'll be pushing out a patch release over the next couple of days with this fix. Thanks again for reporting this...much appreciated! 🍻

@cnoon cnoon closed this as completed Dec 11, 2015
@cnoon cnoon added this to the 2.2.0 milestone Dec 11, 2015
@cnoon
Copy link
Member

cnoon commented Dec 11, 2015

cc @kcharwood for visibility

@cnoon
Copy link
Member

cnoon commented Dec 11, 2015

Hmmmm...found another flaw in this approach. Reopening.

@cnoon cnoon reopened this Dec 11, 2015
@cnoon
Copy link
Member

cnoon commented Dec 11, 2015

Okay, fixed the last remaining issue in a535fff. We cannot store the currentDownloadID at the class level. It needs to be a per-instance level property. Therefore, I moved it to use another associated value on the UIImageView extension.

@MartinChan1
Copy link
Author

Great! thanks @cnoon

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

No branches or pull requests

2 participants