Fix Race conditions#359
Conversation
WalkthroughIntroduces thread-safe storage and invocation for image loading callbacks in ImageManager by adding a private callback queue and backing closures. Public callback properties become computed with synchronized get/set. Progress and completion now capture local callback references and dispatch to the main thread, minimizing race conditions. No functional flow changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant View
participant ImageManager
participant SDWebImage
participant CallbackQueue as Callback Queue
participant MainThread
View->>ImageManager: set success/failure/progress blocks
ImageManager->>CallbackQueue: sync set _success/_failure/_progress
View->>ImageManager: load(url)
ImageManager->>SDWebImage: start request
SDWebImage-->>ImageManager: progress(received,total)
ImageManager->>CallbackQueue: sync get _progress
CallbackQueue-->>ImageManager: progressCallback
alt progressCallback exists
ImageManager->>MainThread: invoke progressCallback(received,total)
end
SDWebImage-->>ImageManager: completion(image,data,cacheType or error)
ImageManager->>CallbackQueue: sync get _success/_failure
CallbackQueue-->>ImageManager: successCallback/failureCallback
alt success
ImageManager->>MainThread: invoke successCallback(image,data,cacheType)
else failure
ImageManager->>MainThread: invoke failureCallback(error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
SDWebImageSwiftUI/Classes/ImageManager.swift (3)
64-84: Nice containment for callback races; consider concurrent queue + barrier writes to reduce contention.Your serial queue works; a lightweight upgrade improves read parallelism without sacrificing safety.
Apply:
- private let callbackQueue = DispatchQueue(label: "ImageManager.callbacks", qos: .userInitiated) + private let callbackQueue = DispatchQueue(label: "ImageManager.callbacks", + qos: .userInitiated, + attributes: .concurrent) @@ var successBlock: ((PlatformImage, Data?, SDImageCacheType) -> Void)? { get { callbackQueue.sync { _successBlock } } - set { callbackQueue.sync { _successBlock = newValue } } + set { callbackQueue.sync(flags: .barrier) { _successBlock = newValue } } } @@ var failureBlock: ((Error) -> Void)? { get { callbackQueue.sync { _failureBlock } } - set { callbackQueue.sync { _failureBlock = newValue } } + set { callbackQueue.sync(flags: .barrier) { _failureBlock = newValue } } } @@ var progressBlock: ((Int, Int) -> Void)? { get { callbackQueue.sync { _progressBlock } } - set { callbackQueue.sync { _progressBlock = newValue } } + set { callbackQueue.sync(flags: .barrier) { _progressBlock = newValue } } }
117-123: Good: snapshot callback then hop to main. Align contract across all callbacks.Progress is delivered on main; success/failure currently aren’t. Consider documenting or making them consistent (see next comment).
Would you like me to add doc comments clarifying callback threading guarantees?
135-152: Dispatch completion callbacks to main and avoid discouragedNSError()init.Prevents UI-thread surprises and fixes SwiftLint discouraged_direct_init (hint at Line 151).
Apply:
- if let image = image { - successCallback?(image, data, cacheType) - } else { - failureCallback?(error ?? NSError()) - } + DispatchQueue.main.async { + if let image = image { + successCallback?(image, data, cacheType) + } else { + let defaultError = NSError( + domain: "SDWebImageSwiftUI.ImageManager", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Image load failed with unknown error"] + ) + failureCallback?(error ?? defaultError) + } + }If existing users rely on background-thread completion, confirm before merging to avoid a breaking change. I can add a minor version note or a feature flag (e.g., deliverCompletionsOnMain).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
SDWebImageSwiftUI/Classes/ImageManager.swift(4 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
SDWebImageSwiftUI/Classes/ImageManager.swift
[Warning] 151-151: Discouraged direct initialization of types that can be harmful
(discouraged_direct_init)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Unit Test (tvOS)
- GitHub Check: Build Library
- GitHub Check: Unit Test (iOS)
- GitHub Check: Cocoapods Lint
- GitHub Check: Run Demo
| var progressBlock: ((Int, Int) -> Void)? | ||
|
|
||
| // Thread-safe callback properties | ||
| private let callbackQueue = DispatchQueue(label: "ImageManager.callbacks", qos: .userInitiated) |
There was a problem hiding this comment.
The ImageManager is not a shared instance. I remember it's just behaved as ObservedObject used for View (it like a ViewModel), so it's created everytime new URL is provided.
Does this (means, you create each DispatchQueue in each Manager instance) cause queue explosion ?
There was a problem hiding this comment.
Valid point!
How do you propose I fix it?
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.
2 participants
I believe this fixes
#358
Summary by CodeRabbit