Fix: dispatch image load callbacks to main thread atomically#364
Fix: dispatch image load callbacks to main thread atomically#364GlennBrann wants to merge 1 commit intoSDWebImage:masterfrom
Conversation
Progress and completion callbacks from SDWebImage are invoked on background threads. Writing ObservableObject-published properties off the main thread is a data race with SwiftUI's rendering. Changes: - Remove per-property didSet dispatches (up to 5 render passes each load) - Move entire progress block body into DispatchQueue.main.async so indicatorStatus.progress is always written on main thread - Move entire completion block body into DispatchQueue.main.async so all property writes are atomic and on the correct thread - Single objectWillChange.send() before all mutations -> one render pass Avoids the per-instance DispatchQueue overhead raised in PR SDWebImage#359 while fixing the same threading bugs more directly.
📝 WalkthroughWalkthroughThe ImageManager refactors its reactive update mechanism by removing property didSet observers and consolidating state updates into the completion closure with explicit main-thread dispatch and transaction handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
SDWebImageSwiftUI/Classes/ImageManager.swift (1)
70-75: Redundantguard let selfbefore dispatch.Line 71's
guard let selfcreates a strong reference that is immediately re-captured weakly by[weak self]on line 74. This pattern is confusing and unnecessary.Remove line 71 and keep only the guard inside the async block:
♻️ Proposed fix
}) { [weak self] (image, data, error, cacheType, finished, _) in - guard let self else { return } // Completion may be called on any thread — always dispatch to main so // withTransaction and all property writes (SwiftUI APIs) are thread-safe. DispatchQueue.main.async { [weak self] in guard let self else { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SDWebImageSwiftUI/Classes/ImageManager.swift` around lines 70 - 75, In the completion closure inside ImageManager.swift remove the outer immediate strong capture check (the first "guard let self" before DispatchQueue.main.async) and rely only on the inner guard in the DispatchQueue.main.async block; ensure the outer closure uses [weak self] in its capture list and the inner async block still uses [weak self] with "guard let self else { return }" so all SwiftUI property writes (in methods like withTransaction or any property mutations inside ImageManager) happen on the main thread and you avoid the redundant strong capture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SDWebImageSwiftUI/Classes/ImageManager.swift`:
- Around line 97-99: The failure path in ImageManager currently passes an empty
NSError() to failureBlock when image is nil and error is nil; replace that with
a meaningful error (either an SDWebImage error constant or a newly constructed
NSError) so callers get diagnostics. In practice, update the else branch that
calls self.failureBlock?(error ?? NSError()) to build and pass a domain-specific
NSError (e.g. domain "SDWebImageSwiftUI.ImageManager", nonzero code, and
NSLocalizedDescriptionKey like "Image is nil but no error returned") or map to
an appropriate SDWebImage error type before invoking failureBlock.
---
Nitpick comments:
In `@SDWebImageSwiftUI/Classes/ImageManager.swift`:
- Around line 70-75: In the completion closure inside ImageManager.swift remove
the outer immediate strong capture check (the first "guard let self" before
DispatchQueue.main.async) and rely only on the inner guard in the
DispatchQueue.main.async block; ensure the outer closure uses [weak self] in its
capture list and the inner async block still uses [weak self] with "guard let
self else { return }" so all SwiftUI property writes (in methods like
withTransaction or any property mutations inside ImageManager) happen on the
main thread and you avoid the redundant strong capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e8b390c-b956-4e8f-b7cb-3ecc1cbff994
📒 Files selected for processing (1)
SDWebImageSwiftUI/Classes/ImageManager.swift
| } else { | ||
| self.failureBlock?(error ?? NSError()) | ||
| } |
There was a problem hiding this comment.
Replace NSError() with a meaningful error.
When image is nil but error is also nil (unexpected edge case), the failure block receives an empty NSError() with no domain, code, or description. This provides no diagnostic value.
Consider creating a domain-specific error or using SDWebImage's error types:
🛠️ Proposed fix
} else {
- self.failureBlock?(error ?? NSError())
+ self.failureBlock?(error ?? NSError(domain: "SDWebImageSwiftUI", code: -1, userInfo: [NSLocalizedDescriptionKey: "Unknown image loading failure"]))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| self.failureBlock?(error ?? NSError()) | |
| } | |
| } else { | |
| self.failureBlock?(error ?? NSError(domain: "SDWebImageSwiftUI", code: -1, userInfo: [NSLocalizedDescriptionKey: "Unknown image loading failure"])) | |
| } |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 98-98: Discouraged direct initialization of types that can be harmful
(discouraged_direct_init)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SDWebImageSwiftUI/Classes/ImageManager.swift` around lines 97 - 99, The
failure path in ImageManager currently passes an empty NSError() to failureBlock
when image is nil and error is nil; replace that with a meaningful error (either
an SDWebImage error constant or a newly constructed NSError) so callers get
diagnostics. In practice, update the else branch that calls
self.failureBlock?(error ?? NSError()) to build and pass a domain-specific
NSError (e.g. domain "SDWebImageSwiftUI.ImageManager", nonzero code, and
NSLocalizedDescriptionKey like "Image is nil but no error returned") or map to
an appropriate SDWebImage error type before invoking failureBlock.
There was a problem hiding this comment.
This was existing so hesitant about changing this.
There was a problem hiding this comment.
@GlennBrann — That's a fair point. Since this code predates the PR, changing it here would be out of scope. Would you like me to open a follow-up issue to track improving the fallback error (and suppress the SwiftLint discouraged_direct_init warning) in a future PR?
(˶˃ ᵕ ˂˶)
|
The only behavior break for exists SDWebImage objc user (I Doubt whether there are), is that The history reason why I hacked there using custom objectChangeHandler also contains this. But, I agree for SwiftUI user, no actual user who want the handle block which is called on the non-main queue. But this progress block behavior changes should also effect some other code in this repo. You can check that And, for So, in conclusion. Please ensure that whatever code that trigger the final user code's progress callback, is exeucted on the main queue. |
2 participants
Problem
SDWebImage invokes progress and completion callbacks on background threads.
ImageManagerwrites to its@Published-equivalent properties directly in those callbacks, which is a data race — SwiftUI readsthese properties on the main thread during rendering.
Additionally, the existing
didSetapproach firesobjectWillChange.send()once per property (up to 5times per load completion), scheduling 5 separate SwiftUI render passes for what should be a single update.
Fix
Move the entire body of both the progress and completion callbacks into
DispatchQueue.main.async.This ensures:
objectWillChange.send()before all mutations collapses them into one render passindicatorStatus.progress(previously written off-main in the progress block) is also safeThe
didSetobservers onimage,imageData,cacheType,error, andisIncrementalare removedsince
objectWillChangeis now sent manually and correctly timed.Relationship to PR #359
PR #359 addresses the same issue but via a per-instance
DispatchQueueon the callback closures.The maintainer raised a valid concern about queue proliferation (one queue per
ImageManager= one perimage cell in a grid). This approach avoids that entirely — no new synchronization primitives are added.
Summary by CodeRabbit
Release Notes
Bug Fixes