Update ObservationImageRepository from Injected to Singleton/Async/Await #169
Conversation
| class ObservationImageRepositoryImpl: ObservationImageRepository, ObservableObject { | ||
|
|
||
| static let shared = ObservationImageRepositoryImpl() |
There was a problem hiding this comment.
Feels like this should be actor, or concurrency protections are needed a level up.
Isn't this the crux of the problem if we're modifying shared state across multiple threads? (See my next comment)
There was a problem hiding this comment.
TLDR, making the shared an actor would make the entire thing and all of it's parts an actor. We don't need or want this. We only care about protecting what needs protecting.
| actor ImageCache { | ||
| private var cache = NSCache<NSString, UIImage>() | ||
|
|
||
| init(limit: Int = 100) { |
There was a problem hiding this comment.
Is there a reason to use actor for ImageCache, instead of the ObservationImageRepositoryImpl?
NSCache is already thread safe, so I'm not sure how this helps to wrap it again.
There was a problem hiding this comment.
Because we do not want the entire repo to be an actor, we choose to make only what is required be an actor. In this case it's the cache itself. It doesn't matter that NSCache is already safe, our ImageCache is not just an NSCache. Besides, this was the entire point, to very clearly identify and manage the cache as an actor
There was a problem hiding this comment.
I'm trying to reason about the change.
Is the point that this actor is enforcing read/write access using the actor mechanism, while the previous was technical thread safe, it wasn't enforcing proper shared access across all CRUD type operations?
There was a problem hiding this comment.
I still question if actor is at the right level. It feels like it's not providing much benefit, when there is shared state in the repository that is unprotected.
| @MainActor | ||
| func imageAtPath(imagePath: String?) async -> UIImage { |
There was a problem hiding this comment.
My only concern is that we're doing background type work on the main thread. If this is truly loading files, it feels like the asynchronous work needs to happen on a background thread.
- I'm having trouble understanding the current Swift 5.10 model vs. 6.2 with upcoming build settings.
- If the image is cached, loading is fast, but if it's not, we need to do a more expensive load option (small files might not be an issue ... is this tile maps or just icons?)
There was a problem hiding this comment.
This work related to UI. It may be related to files, but it's directly related to updating the UI.
There was a problem hiding this comment.
Doing the image file loading on the main thread is going to be problematic. That should be in the background.
| guard let path = iconPath else { return } | ||
| let image = await ObservationImageRepositoryImpl.shared.imageAtPath(imagePath: path) | ||
| uiImage = image |
There was a problem hiding this comment.
Can we inject the shared instance like you do below?
var imageRepository: ObservationImageRepository
init(imageRepository: ObservationImageRepository = ObservationImageRepositoryImpl.shared /* Other parameters */) {
// ...
}
| if let image = UIImage(contentsOfFile: resolvedPath), let cgImage = image.cgImage { | ||
| let scale = image.size.width / annotationScaleWidth | ||
| let scaledImage = UIImage(cgImage: cgImage, scale: scale, orientation: image.imageOrientation) | ||
| imageCache.setObject(scaledImage, forKey: cacheKey) | ||
| await cache.set(scaledImage, for: cacheKey) | ||
| scaledImage.accessibilityIdentifier = resolvedPath | ||
| return scaledImage | ||
| } |
There was a problem hiding this comment.
Here we're going to do I/O on the main thread, so I think we'd need to bump it to the background in a fashion like this:
// I/O and resizing are expensive operations
let loadedImage: UIImage? = await Task.detached {
guard let image = UIImage(contentsOfFile: resolvedPath),
let cgImage = image.cgImage else {
return nil
}
let scale = image.size.width / annotationScaleWidth
return UIImage(cgImage: cgImage, scale: scale, orientation: image.imageOrientation)
}.value // Await the result of the background work
There was a problem hiding this comment.
I think this is on the right track for the @Injected replacement, but I think we need to rework the Swift Concurrency logic more to make this safe (and non-blocking). The image loading is going to block the main thread as it is written.
I'm still wrestling with how we should be adopting Swift 6.2 and better concurrency features. Those warnings, if enabled might help us focus on key problems at compile time (whack-a-mole).
3 participants
ObservationImageRepository upgrade
@injected...Profiled