iOS Table View stuck in loop while trying to cache images

Question

I am using async to get my images from an xml. Parser is working correctly and I can output the URLs. In the async I am trying to cache the images to a mutable dictionary. I am stuck in a loop and the images will not output at all any more. Here is my code that I am stuck on.

    - (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
    {

        NSLog(@"Got Here");

        static NSString *CellIdentifier = @"NewsCell";
        NewsCell *cell = [tableView dequeueReusableCellWithIdentifier:CellIdentifier];

        NSLog(@"Got Here 2");

        // Configure the cell...

        NewsItem *item = [newsItemsArray objectAtIndex:indexPath.row];

        cell.newsTitle.text = item.title;

        NSLog(@"Got Here 3");

        NSMutableDictionary *record = [_records objectAtIndex:[indexPath row]];
        if ([record valueForKey:@"actualImage"]) {
            NSLog(@"Record Found");
            [cell.newsImage setImage:[record valueForKey:@"actualImage"]];
        } else {
            NSLog(@"Record Not Found");
            dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT,(unsigned long)NULL), ^(void)
                   {
                       NSLog(item.image);
                       NSData *imageData = [NSData dataWithContentsOfURL:[NSURL URLWithString:item.image]];
                       dispatch_async(dispatch_get_main_queue(), ^(void){
                           [record setValue:[UIImage imageWithData:imageData] forKey:@"actualImage"];
                           [cell.newsImage setImage:[record valueForKey:@"actualImage"]];
                           if (tableView) {
                               [tableView reloadRowsAtIndexPaths:[NSArray arrayWithObject:indexPath] withRowAnimation:UITableViewRowAnimationNone];
                           }
                       });
                   });
        }

        return cell;
    }

Thanks in advance for you help.

why are you using NSMutableDictonary for Caching ??? This is not the way caching works — Kunal Balani, Oct 10 '13 at 18:16
Use SDImage Cache library or NSCache at worst. For godsake don't do such things.. You have limited heap memory. — Kunal Balani, Oct 10 '13 at 18:17
I am only using NSMutableDictionay for caching because there are only about ten images that I will caching and for a short amount of time. Is the SDImage Cache library still more beneficial to use? And do you know of an example of how to use SDImage Cache library so I can look into this further? — mcphersonjr, Oct 10 '13 at 18:44

score 0 · Answer 1 · edited May 23 '17 at 12:10

You may try the following code. You should also research your problem and try to find other existing answers and explanations. This is a very frequent question, countless times answered on SO ;)

For example: loading images in table using blocks

The code tries to fix the most obvious issues, but there is no guarantee that it works in all edge cases. You need to debug and test.

The main problem with the original code is, that it tries to set the cell's newsImage property within the completion block. When that gets executed, the captured cell is likely not be associated to the same row anymore. Remember: cells will be reused!

Thus, in the completion block, the changed code re-evaluates the corresponding cell, from the indexPath which has been captured at the start of the block:

NewsCell* cell2 = [tableView cellForRowAtIndexPath:indexPath]; // cell equals nil, if not visible

The returned cell may be not visible, in which case the table view returns nil.

Basically, the same is applied to the record variable. Just for different reasons: the record variable is a variable with automatic scope, that is it gets released when the method returns. Capturing it in the block is probably not a good idea.

Thus, it needs to be re-evaluted in the block. Thats basically a short hand for [self.records objectAtIndex:[indexPath row]];

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
    static NSString *CellIdentifier = @"NewsCell";

    NewsCell *cell = [tableView dequeueReusableCellWithIdentifier:CellIdentifier];
    // Configure the cell...

    NewsItem *item = [newsItemsArray objectAtIndex:indexPath.row];
    cell.newsTitle.text = item.title;
    NSMutableDictionary *record = [_records objectAtIndex:[indexPath row]];
    if ([record valueForKey:@"actualImage"]) {
        NSLog(@"Record Found");
        [cell.newsImage setImage:[record valueForKey:@"actualImage"]];
    } else {
        NSLog(@"Record Not Found");
        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT,(unsigned long)NULL), ^(void)
               {
                   NSLog(item.image);
                   NSData *imageData = [NSData dataWithContentsOfURL:[NSURL URLWithString:item.image]];
                   dispatch_async(dispatch_get_main_queue(), ^(void){
                       NSMutableDictionary *record2 = [_records objectAtIndex:[indexPath row]];
                       [record2 setValue:[UIImage imageWithData:imageData] 
                                                                   forKey:@"actualImage"];
                       NewsCell* cell2 = [tableView cellForRowAtIndexPath:indexPath]; // cell equals nil, if not visible
                       if (cell2) {
                           [cell2.newsImage setImage:[record2 valueForKey:@"actualImage"]];
                           [tableView reloadRowsAtIndexPaths:[NSArray arrayWithObject:indexPath] withRowAnimation:UITableViewRowAnimationNone];
                        }
                   });
               });
    }

    return cell;
}

Any specific reason why your code should work? and what is the difference ?? — Kunal Balani, Oct 10 '13 at 18:26
Also, if this is asked countless number of time please give URL to atleast one question. — Kunal Balani, Oct 10 '13 at 18:29
I don't think so because block retains everything referenced inside it. Block should retain the cell as well. — Kunal Balani, Oct 10 '13 at 18:39
You must re-evaluate the cell. The _record_ variable *may* be captured. But if it is, it may expose a concurrency problem. It's just safer to re-evalute it on the main thread. — CouchDeveloper, Oct 10 '13 at 18:44
As mentioned, a cell is *reused*. When the completion block finally gets executed, the cell may be associated to a different row in the meantime. Thus, there's no guarantee that the cell is still displaying contents for the row at _indexPath_. In order to get the actual cell at indexPath you need to ask the table view again. — CouchDeveloper, Oct 10 '13 at 19:01
@KunalBalani Added link for another question with quite a similar answer. — CouchDeveloper, Oct 10 '13 at 19:27

jsd · Answer 2 · 2013-10-10T18:48:14.630

Create a UITableViewCell subclass. When you need to load an image in it, create an NSOperation subclass which does the actual network connection. (see http://www.dribin.org/dave/blog/archives/2009/05/05/concurrent_operations/ for an example). Alternatively, use a 3rd party package which handles this, there are millions of them. My current favorite is MKNetworkKit. Store a reference to your operation. In the cell's prepareForReuse, cancel the connection if it hasn't completed. This will prevent your cell getting the wrong image when scrolling if the first request completes after the second (happens more often then you'd think).

I would like to avoid using 3rd party programing as much as possible. Thanks for the NSOperation idea. — mcphersonjr, Oct 10 '13 at 18:53

Kunal Balani · Answer 3 · 2013-10-10T18:54:39.820

First of all replace

[record setValue:[UIImage imageWithData:imageData] forKey:@"actualImage"];

with

[record setObject:[UIImage imageWithData:imageData] forKey:@"actualImage"];

and your code should work. Since the dictionary is always empty it always goes to else block. Print NSMutableDictionary and you will realize this.

This is not the way caching is done. It's not only about memory but also about coding practices.

Please use a clean approach like NSCache or SDWebImage

SDWebImage takes care of caching as well. Use NSOperation to make request rather than GCD. to avoid nested blocks and concerrency issues. Here is how a clean code looks like

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
    static NSString *CellIdentifier = @"cell";
    NewsCell *cell = [tableView dequeueReusableCellWithIdentifier:CellIdentifier forIndexPath:indexPath];

    NewsItem *item = [newsItemsArray objectAtIndex:indexPath.row];

    cell.newsTitle.text = item.title;
     // category from library
    [cell.imageView setImageWithURL:newsItem.imageURL];

    if ([item needsRefresh] )
    {
         cell.newsTitle.text = item.title;

        NewsOperation *operation = [[NewsOperation alloc] initForNewsItem:newItem completion:^(NewsItem *newsItem, NSError *error) {
            if (newsItem)
            {
                [tableView reloadRowsAtIndexPaths:@[indexPath] withRowAnimation:UITableViewRowAnimationNone];
            }
        }];
        [[self newsItemQueue] addOperation:operation];
    }

    return cell;
}

Could you send me a link to a tutorial you might know of or more information about NSCache or SDWebImage. Thanks — mcphersonjr, Oct 10 '13 at 18:59
@Cedric IMHO, it's more important to fix the dominant issues. You can improve caching at a later point. On the other hand, SDWebImage makes it possible to implement a solution in a less elaborated way. Eventually, I would go for it. ;) — CouchDeveloper, Oct 10 '13 at 19:07

iOS Table View stuck in loop while trying to cache images

3 Answers3