Do not execute the block if task is cancelled

Question

I have MainViewController -> ThumbnailViewController -> ImageFullScreenViewController. As their name implies, I have a main screen from which I go to a screen which shows collection of images and on selecting an image, I open the image in full screen.

In ThumbnailViewController, I download images as follows

private func getImages() {
        self.galleryImages.removeAll()
        for url in urls {
            let task = NSURLSession.sharedSession().dataTaskWithURL(url) { (data, response, error) in

                // errors are handled

                if let image = UIImage(data: data) {
                    self.galleryImages.append(image!)
                }
            }
            task.resume()
            requests.append(task)
        }
    }

In viewDidLoad() I call getImages(). In viewWillDisappear() of ThumbnailViewController, I cancel the ongoing tasks.

override func viewWillDisappear(animated: Bool) {
    super.viewWillDisappear(animated)
    if isMovingFromParentViewController() {
        if requests.count > 0 {
            for request in requests {
                if request is NSURLSessionDataTask {
                    let task = request as! NSURLSessionDataTask
                    task.cancel()
                }
            }
        }
    }
}

The problem is, when I open ThumbnailViewController and immediately go back and if I open ThumbnailViewController immediately, I can see two copies of same image in some cases (rarely, but reproducible).

On investigation, I found that, cancelling the NSURLSessionDataTask in viewWillDisappear does cancel the task only, but not the completion block (which is the behavior). In some rare cases, the completion block is executed for the previous NSURLSessionDataTask thereby ending mixing with the response of the new NSURLSessionDataTask.

How could I handle this?

Note: galleryImages is a singleton property which I am reusing in ImageFullScreenViewController (UIPageViewController)

On a easier side :) why don't you simply check if image exists before adding it to galleryImages using something like galleryImages.contains( image) { ... } — Sandeep Bhandari, Sep 16 '16 at 09:29
Is `galleryImages` computed property or do it have any observers? Please, show declaration. There is nothing strange, that task sometimes already on the way to finish with completion; strange is how it able to mix received image with array of images in *new* instance of `ThumbnailViewController`. This can happend if `galleryImages` shared somehow. — Yury, Sep 16 '16 at 09:43
Sorry. I should have mentioned. `galleryImages` is a singleton. I made it singleton, because, I am reusing the same array for the `UIPageViewController` in `ImageFullScreenViewController` — iOS, Sep 16 '16 at 10:04

score 0 · Accepted Answer · answered Sep 16 '16 at 10:15

0

Firstly, you should take care about release ThumbnailViewController object. Declare capture list with weak self, and it will, by additional, remove your issue in 99% cases (if you are not retaining self in some other places), though your model still not perfect with that singletone

private func getImages() {
    self.galleryImages.removeAll()
    for url in urls {
        let task = NSURLSession.sharedSession().dataTaskWithURL(url) { [weak self] (data, response, error) in

            // errors are handled
            // replace self. with self?.

            if let image = UIImage(data: data) {
                self?.galleryImages.append(image!)
            }
        }
        task.resume()
        requests.append(task)
    }
}

answered Sep 16 '16 at 10:15

Yury

6,044
3
19
41

1

Revising the model as suggested solved the problem. Thank you! – iOS Sep 16 '16 at 11:10
Unless I misunderstood your suggestion, even that isn't guaranteed to work correctly if a single view controller accidentally starts two fetches concurrently somehow. IMO, a more robust approach would be to A. replace galleryImages with a new array in removeAll, B. store that in a new variable *outside* the block, C. capture that variable inside the block instead of self.galleryImages, D. run the guts of the block on your main thread so you won't have multiple threads mucking with a single mutable array. That way, each set of requests always fills a new array and only the latest gets used. – dgatwood Sep 17 '16 at 06:13
@dgatwood you are right. I just pointed at important issue on existed code, and at unperfect model; I decided that suggesting concrete other model will be too broad – Yury Sep 17 '16 at 09:48

Do not execute the block if task is cancelled

1 Answers1