Skip to content
New issue

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

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

Already on GitHub? # to your account

af_setImage race condition when runImageTransitionIfCached=true #293

Closed
1 task done
RonaldAndersonAol opened this issue Sep 25, 2017 · 1 comment
Closed
1 task done
Assignees
Milestone

Comments

@RonaldAndersonAol
Copy link

What did you do?

Used af_setImage extension function on UIImageView, with runImageTransitionIfCached=true. When the image is found in the cache, the transition is run on a delayed (0.001 sec) GCD block. This is fine, except for when the image request is canceled with a call to af_cancelImageRequest. The subsequent new image request may get overridden with the delayed block, if the next request also fetches the image from the cache, but has runImageTransitionIfCached=false. Then the "cancelled" cached image will overwrite the new cached image.

Proposed solution:

UIImageView+AlamofireImage.swift : line 287

OLD:

if runImageTransitionIfCached {
let tinyDelay = DispatchTime.now() + Double(Int64(0.001 * Float(NSEC_PER_SEC))) / Double(NSEC_PER_SEC)
// Need to let the runloop cycle for the placeholder image to take affect
DispatchQueue.main.asyncAfter(deadline: tinyDelay) { [weak self] in
self.run(imageTransition, with: image)
completion?(response)
}
}

NEW:

if runImageTransitionIfCached {
if let placeholderImage = placeholderImage { self.image = placeholderImage }

 self.run(imageTransition, with: image)
 completion?(response)

}

What did you expect to happen?

The second image request should not get overwritten with the cancelled image.

What happened instead?

The first cancelled image overwrote the subsequent image request image.

Alamofire Environment

Alamofire version: 4.5.1
Xcode version: 9.0 (9A235)
Swift version: 4.0
Platform(s) running AlamofireImage: tvOS
macOS version running Xcode: 10.12.6

Demo Project

None

RonaldAndersonAol added a commit to RonaldAndersonAol/AlamofireImage that referenced this issue Sep 26, 2017
Image transition for cached image hit does not need to be called as a delayed async GCD block. It just needs to set the placeholder image before running the image transition. If the delayed block is used, then if the image request is cancelled, the block is not cancelled. And a subsequent new image request that has no image transition and also hits the cache will be possibly overwritten with the delayed block, causing the wrong image to be displayed. This issue is resolved with the code change.
@cnoon cnoon self-assigned this Feb 22, 2020
cnoon added a commit that referenced this issue Feb 23, 2020
This change contains two important updates. The first is to set the placeholder image when loading from the cache with runImageTransitionIfCached enabled. This makes the behavior between loading from the cache and downloading from the network exactly the same. If you don’t do this, you can really tell the difference between cache and remote loads which we don’t want to do.

I also added a check to make sure the receipt hasn’t changed out from under us before running the transition. This is in regards to Issue #293 which I cannot validate. You must perform the queue hop to allow the image view to be redisplayed before running the animation. If you don’t do this, the image isn’t reset before running the transition which can lead to the initial display of the old image rather than the placeholder or no image. The queue hop MUST be done in order to ensure the image view is redrawn before the transition begins.
@cnoon
Copy link
Member

cnoon commented Feb 23, 2020

Closing this issue out for PR #294. Please direct all further comments to that PR. Thanks!

@cnoon cnoon closed this as completed Feb 23, 2020
@cnoon cnoon added this to the 4.0.0 milestone Feb 23, 2020
ryndinol pushed a commit to ryndinol/AlamofireImage that referenced this issue Apr 26, 2021
…fire#293)

This change contains two important updates. The first is to set the placeholder image when loading from the cache with runImageTransitionIfCached enabled. This makes the behavior between loading from the cache and downloading from the network exactly the same. If you don’t do this, you can really tell the difference between cache and remote loads which we don’t want to do.

I also added a check to make sure the receipt hasn’t changed out from under us before running the transition. This is in regards to Issue Alamofire#293 which I cannot validate. You must perform the queue hop to allow the image view to be redisplayed before running the animation. If you don’t do this, the image isn’t reset before running the transition which can lead to the initial display of the old image rather than the placeholder or no image. The queue hop MUST be done in order to ensure the image view is redrawn before the transition begins.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants