Safe way to download content from a list of URL's in Swift 3

Question

I have a list of URL's that provide a location to an Image resource. I managed to find a solution, but I feel as if there is a much better way than the procedure shown in the code below. Any tips on how to improve this method to retrieve images asynchronously is greatly appreciated!

Isn't something weird about calling the completionHandler after I append each item, AND adding 1 to index (i), outside of async block, meaning that the while loop iterates to the next url item before the last url has been fully handled??

typealias imagesHandler = (_ images: [UIImage]) -> Void

func fetchImages(forUser user: User, completionHandler: imagesHandler?) {

    var images = [UIImage]()

    self.fetchImageUrlList(forUser: user) { (urlList: [URL]) in

        var i = 0

        while i <= urlList.count - 1 {
            let request = URLRequest(url: urlList[i])
            let dataTask = URLSession(configuration: .default).dataTask(with: request, completionHandler: {
                (data: Data?, response: URLResponse?, error: Error?) in
                guard error == nil else { return }
                guard let data = data else { return }
                guard let image = UIImage(data: data) else { return }

                images.append(image)
                completionHandler?(Array(Set(images)))
            })
            i += 1
            dataTask.resume()
        }
    }
}

Luca Angeletti · Accepted Answer · 2018-03-13T11:43:27.060

Grand Central Dispatch

This is a typical multithreading scenario where Grand Central Dispatch is very helpful.

First of all here's your code updated in order to use Grand Central Dispatch

func fetchImages(forUser user: User, completionHandler: ImagesHandler?) {

    var images = [UIImage]()
    let group = DispatchGroup()
    let serialQueue = DispatchQueue(label: "serialQueue")

    fetchImageUrlList(forUser: user) { urls in

        urls.forEach { url in
            // ***********************************************
            // tells the group that there is a pending process
            group.enter()
            URLSession(configuration: .default).dataTask(with: url) { (data, response, error) in

                guard let data = data, let image = UIImage(data: data), error == nil else { group.leave(); return }

                // ***************************************************************************
                // creates a synchronized access to the images array
                serialQueue.async {
                    images.append(image)

                    // ****************************************************
                    // tells the group a pending process has been completed
                    group.leave()
                }
            }.resume()
        }

        group.notify(queue: .main) {
            // *****************************************************************************************
            // this will be executed when for each group.enter() call, a group.leave() has been executed
            completionHandler?(images)
        }
    }
}

GroupDispatch

With this line I create a GroupDispatch

let group = DispatchGroup()

You can see it as a counter. Every time you call group.enter() the counter is increased by 1. And every time you call group.leave() the counter is decreased by 1.

Of course DispatchGroup is thread safe.

Finally everything you write

group.notify(queue: .main) {
    // HERE <------
}

will be executed when the internal counter of DispatchGroup is zero. Infact it means all the pending "operations" are completed.

Serial Queue

Finally you need to access the images array from different threads. This is very risky.

So we use a Serial Queue.

serialQueue.async {
    images.append(image)
    group.leave()
}

All the closure we append to a Serial Queue are executed 1 at the time. So the images array is access safely.

I haven't tested the code, please try and let me know ;)

Wow thanks for providing such a thoughtful answer. Since I'm working with firebase, my code is riddled with nested completion blocks, so this a nice way to get my feet wet with GCD. I will test it out and get back to you! — , Jun 01 '17 at 22:33
Not related to the question but considering that they are all images (only GET). There is no need to map the urls into URLRequest objects. You can just pass the urls for the dataTask. — Leo Dabus, Jun 01 '17 at 23:53
`URLSession(configuration: .default).dataTask(with: url) { data, response, error in // ...` — Leo Dabus, Jun 01 '17 at 23:55
@LeoDabus Good point. I'll update my code accordingly. Thanks — Luca Angeletti, Jun 02 '17 at 11:45
@LucaAngeletti, your answer has been EXTREMELY useful. I have an array of URL strings. Each URL must be downloaded, the contents extracted, certain information must be saved and if there are any image links, they need to be downloaded, after which everything must be saved as an object and uploaded to a server. This is EXACTLY what I was looking for, thank you so much for your help. This information is priceless! — WhiteEagle, Nov 07 '21 at 18:32

score 0 · Answer 2 · answered Jun 01 '17 at 20:53

If you want to make all three requests simultaneously (which is fine), then you would increment i as you are (since that is looping through the urlList array).

The issue you could have is thread contention on the completions. Appending to the images array is not an atomic operation and I don't know if those completions are all happening on the same thread. You could construct the URLSession with a delegate queue that has only one thread servicing it -- that would ensure the completions where coordinated. If not, you might need to synchronize the update to images.

Also, you are calling the fetchImages completionHandler once for each completed image (not sure you want that), not once at the end when they are all done.

Safe way to download content from a list of URL's in Swift 3

2 Answers2

Grand Central Dispatch

GroupDispatch

Serial Queue