JeffG JeffG - 5 months ago 30
iOS Question

Using dispatch_async to load images in background

I am trying to update a progress bar as my images load. I've read several answers here and tried formatting my code many different ways. I'm trying to read in the images and update the progress bar. Then, will all the images are loaded call the code to process them. The best result I've got was some code that works most of the time. However, if I'm dealing with a situation where it is pulling in a lot of images, I get weird errors. I think it is going ahead and running the continue code before all the images are fully loaded. When I remove the dispatch_async, the code works fine but the progress bar does not update.

func imageLocXML(il:String) {
dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0)) {
let url:NSURL? = NSURL(string: il)
let data:NSData? = NSData(contentsOfURL: url!)
let image = UIImage(data: data!)
pieceImages.append(image!)
self.numImagesLoaded += 1
self.updateProgressBar()
if self.numImagesLoaded == self.numImagesToLoad {
self.continueLoad()
}
}
}

Rob Rob
Answer

There a number of issues:

  1. This code isn't thread safe, because you have race condition on numImagesLoaded. This could, theoretically, result in continueLoad to be called more than once. You can achieve thread safety by synchronizing numImagesLoaded by dispatching updates to this (and other model objects) back to the main queue.

  2. Like DashAndRest said, you have to dispatch the UI update to the main queue, as well.

  3. When you made this asynchronous, you introduced a network timeout risk when you initiate a lot of requests. You can solve this by refactoring the code to use operation queues instead of dispatch queues and specify maxConcurrentOperationCount.

  4. The images are being added to an array:

    • Because these tasks run asynchronously, they're not guaranteed to complete in any particular order, and thus the array won't be in order. You should save the images in a dictionary, in which case the order no longer matters.

    • Just like numImagesLoaded, the pieceImages isn't thread safe.

  5. You are using a lot of forced unwrapping, so if any requests failed, this would crash.

But to address this, we have to step back and look at the routine calling this method. Let's imagine that you have something like:

var pieceImages = [UIImage()]

func loadAllImages() {
    for imageUrl in imageURLs {
        imageLocXML(imageUrl)
    }
}

func imageLocXML(il:String) {
    dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0)) {
        let url:NSURL? = NSURL(string: il)
        let data:NSData? = NSData(contentsOfURL: url!)
        let image = UIImage(data: data!)
        self.pieceImages.append(image!)
        self.numImagesLoaded += 1
        self.updateProgressBar()
        if self.numImagesLoaded == self.numImagesToLoad {
            self.continueLoad()
        }
    }
}

I'm suggesting that you replace that with something like:

var pieceImages = [String: UIImage]()

func loadAllImages() {
    let queue = NSOperationQueue()
    queue.maxConcurrentOperationCount = 4

    let completionOperation = NSBlockOperation {
        self.continueLoad()
    }

    for imageURL in imageURLs {
        let operation = NSBlockOperation() {
            if let url = NSURL(string: imageURL), let data = NSData(contentsOfURL: url), let image = UIImage(data: data) {
                NSOperationQueue.mainQueue().addOperationWithBlock {
                    self.numImagesLoaded += 1
                    self.pieceImages[imageURL] = image
                    self.updateProgressBar()
                }
            }
        }

        queue.addOperation(operation)
        completionOperation.addDependency(operation)
    }

    NSOperationQueue.mainQueue().addOperation(completionOperation)
}

Having said that, I think there are deeper issues here:

  • Should be loading images in advance like this at all? We would generally advise lazy loading of images, only loading them when needed.

  • If you're going to load images into a structure like this, you should gracefully handle memory pressure, purging it upon memory pressure. You then need to gracefully handle what to do when you go to retrieve an image and it's been purged due to memory pressure (leading you right back to a just-in-time lazy loading pattern).

  • We'd generally advise against synchronous network requests (the NSData(contentsOfURL:_)). We'd generally use NSURLSession which is cancellable, offers richer error handling, etc. Admittedly, that complicates the above code even further (probably leading me down the road of asynchronous NSOperation subclass), but you should at least be aware of the limitations of contentsOfURL.