Skip to content

Fix: dispatch image load callbacks to main thread atomically #364

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.

Sign up for GitHub

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

Open
GlennBrann wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix: dispatch image load callbacks to main thread atomically #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
260b16b
Fix: dispatch image load callbacks to main thread atomically
Mar 6, 2026
f442952
Fix: also dispatch AnimatedImage progress callback to main thread
Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SDWebImageSwiftUI/Classes/AnimatedImage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,8 @@ public struct AnimatedImage : PlatformViewRepresentable {
}
DispatchQueue.main.async {
context.coordinator.imageLoading.progress = progress
self.imageHandler.progressBlock?(receivedSize, expectedSize)
}
self.imageHandler.progressBlock?(receivedSize, expectedSize)
}) { (image, data, error, cacheType, finished, _) in
context.coordinator.imageLoading.image = image
context.coordinator.imageLoading.isLoading = false
Expand Down
116 changes: 44 additions & 72 deletions SDWebImageSwiftUI/Classes/ImageManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,45 +15,15 @@ import SDWebImage
@available(iOS 14.0, macOS 11.0, tvOS 14.0, watchOS 7.0, *)
public final class ImageManager : ObservableObject {
/// loaded image, note when progressive loading, this will published multiple times with different partial image
public var image: PlatformImage? {
didSet {
DispatchQueue.main.async {
self.objectWillChange.send()
}
}
}
public var image: PlatformImage?
/// loaded image data, may be nil if hit from memory cache. This will only published once even on incremental image loading
public var imageData: Data? {
didSet {
DispatchQueue.main.async {
self.objectWillChange.send()
}
}
}
public var imageData: Data?
/// loaded image cache type, .none means from network
public var cacheType: SDImageCacheType = .none {
didSet {
DispatchQueue.main.async {
self.objectWillChange.send()
}
}
}
public var cacheType: SDImageCacheType = .none
/// loading error, you can grab the error code and reason listed in `SDWebImageErrorDomain`, to provide a user interface about the error reason
public var error: Error? {
didSet {
DispatchQueue.main.async {
self.objectWillChange.send()
}
}
}
public var error: Error?
/// true means during incremental loading
public var isIncremental: Bool = false {
didSet {
DispatchQueue.main.async {
self.objectWillChange.send()
}
}
}
public var isIncremental: Bool = false
/// A observed object to pass through the image manager loading status to indicator
public var indicatorStatus = IndicatorStatus()

Expand Down Expand Up @@ -85,46 +55,48 @@ public final class ImageManager : ObservableObject {
self.indicatorStatus.isLoading = true
self.indicatorStatus.progress = 0
currentOperation = manager.loadImage(with: url, options: options, context: context, progress: { [weak self] (receivedSize, expectedSize, _) in
// This block may be called in non-main thread
guard let self = self else {
return
}
let progress: Double
if (expectedSize > 0) {
progress = Double(receivedSize) / Double(expectedSize)
} else {
progress = 0
}
self.indicatorStatus.progress = progress
if let progressBlock = self.progressBlock {
DispatchQueue.main.async {
progressBlock(receivedSize, expectedSize)
// This block may be called in non-main thread — dispatch to main for thread safety
DispatchQueue.main.async { [weak self] in
guard let self else { return }
let progress: Double
if (expectedSize > 0) {
progress = Double(receivedSize) / Double(expectedSize)
} else {
progress = 0
}
self.indicatorStatus.progress = progress
self.progressBlock?(receivedSize, expectedSize)
}
}) { [weak self] (image, data, error, cacheType, finished, _) in
guard let self = self else {
return
}
if let error = error as? SDWebImageError, error.code == .cancelled {
// Ignore user cancelled
// There are race condition when quick scroll
// Indicator modifier disapper and trigger `WebImage.body`
// So previous View struct call `onDisappear` and cancel the currentOperation
return
}
withTransaction(self.transaction) {
self.image = image
self.error = error
self.isIncremental = !finished
if finished {
self.imageData = data
self.cacheType = cacheType
self.indicatorStatus.isLoading = false
self.indicatorStatus.progress = 1
if let image = image {
self.successBlock?(image, data, cacheType)
} else {
self.failureBlock?(error ?? NSError())
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 }
if let error = error as? SDWebImageError, error.code == .cancelled {
// Ignore user cancelled
// There are race condition when quick scroll
// Indicator modifier disapper and trigger `WebImage.body`
// So previous View struct call `onDisappear` and cancel the currentOperation
return
}
// Send once, before any mutation — correct ObservableObject semantics.
// This collapses all property changes into a single SwiftUI render pass.
self.objectWillChange.send()
withTransaction(self.transaction) {
self.image = image
self.error = error
self.isIncremental = !finished
if finished {
self.imageData = data
self.cacheType = cacheType
self.indicatorStatus.isLoading = false
self.indicatorStatus.progress = 1
if let image = image {
self.successBlock?(image, data, cacheType)
} else {
self.failureBlock?(error ?? NSError())
}
Comment on lines +97 to +99
Copy link

@coderabbitai coderabbitai bot Mar 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
} 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.

Copy link
Author

@GlennBrann GlennBrann Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was existing so hesitant about changing this.

Copy link

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

(˶˃ ᵕ ˂˶)

}
}
}
Expand Down
Loading