Release the image view while its download is still in flight - #2561
Merged
onevcat merged 1 commit intoJul 25, 2026
Merged
Conversation
`setImage(with:...)` handed closures to `KingfisherManager.retrieveImage` that captured `self` (the `KingfisherWrapper`), which references the image view strongly. An in-flight download therefore kept the view alive until it finished, so a view whose owner had already been released (for example, an image view inside a popped view controller) was deallocated late. Hold the view weakly in the download callbacks through a small `WeakBox`. When the view is already gone as a callback runs, the UI update is skipped and the result is still forwarded to the caller. The download keeps running and still populates the cache, so behavior is unchanged while the view is alive. Closes onevcat#2313
onevpaw
reviewed
Jul 25, 2026
onevpaw
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the exact head: the weak capture cleanly removes the ImageView retention path while preserving download, cache, and UI-update behavior. The targeted iOS and macOS tests pass.
Optional follow-up: add a regression test confirming that completionHandler is still invoked after the ImageView has been released. This is not a blocking issue.
The workflows observed during review still required approval before producing jobs, so I am leaving a comment rather than an approval.
onevpaw - an assistant to @onevcat
Owner
|
@devzahirul Make sense! Thank you for the fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes #2313.
imageView.kf.setImage(with:)hands several closures toKingfisherManager.retrieveImage(downloadTaskUpdated,progressiveImageSetter,completionHandler). Each of them capturesself, which is aKingfisherWrappervalue that references the target image view strongly. Those closures live on the in-flightSessionDataTaskuntil the download finishes, so the image view is kept alive until the request completes — even after its owner has been released.In practice, an image view inside a view controller that is popped/dismissed while an image is still downloading is not deallocated until the download finishes (or until
cancelDownloadTask()is called manually, as suggested in the issue thread).Fix
Hold the image view weakly inside those callbacks through a tiny
WeakBox:downloadTaskUpdated/progressiveImageSetterbecome no-ops once the view is gone.completionHandlerskips all UI work when the view is gone, but still forwards the result to the caller so anyone awaiting completion is not left hanging.The download itself is untouched — it keeps running and still populates the cache — and the behavior is identical while the view is alive. This only removes Kingfisher's own retention of the view; there is no public API change.
Scope note: this PR addresses the
UIImageView/NSImageViewpath reported in #2313. The button /CPListItem/NSTextAttachmentpaths inHasImageComponent+Kingfisher.swiftuse an accessor-closure indirection and can get the same treatment in a follow-up; I kept this PR focused per the contribution guidelines.Tests
Added
testImageViewNotRetainedByInFlightDownloadinImageViewExtensionTests:weakreference becomesnil).I confirmed the test fails on
master(the view is retained) and passes with this change. The fullImageViewExtensionTestssuite (35 tests) passes on the iOS simulator.Why it matters
Keeping a view alive for the duration of a download is surprising and can noticeably increase peak memory in image-heavy, fast-scrolling screens where cells/controllers are torn down before their images arrive. Capturing the view weakly matches the common expectation (and the behavior of other image libraries) that setting an image should not extend the view's lifetime.