Using Singleton synchronized array with NSThread

Question

I have a books app with a UISearchBar, where the user types any book name and gets search results (from ext API call) below as he types.

I am using a singleton variable in my app called retrievedArray which stores all the books.

@interface Shared : NSObject {
    NSMutableArray *books;
}

@property (nonatomic, retain) NSMutableArray *books;

+ (id)sharedManager;

@end

This is accessed in multiple .m files using NSMutableArray *retrievedArray; ...in the header file

retrievedArray = [[Shared sharedManager] books];

My question is how do I ensure that the values inside retrievedArray remain synchronized across all the classes.

Actually the values inside retrievedArray gets added through an NSXMLParser (i.e. through external web service API). There is a separate XMLParser.m file, where I do all the parsing and fill the array. The parsing is done on a separate thread.

    - (void) run: (id) param  {
        NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];

        NSXMLParser *parser = [[NSXMLParser alloc] initWithContentsOfURL: [self URL]];
        [parser setDelegate: self];
    [parser parse];
        [parser release];

        NSString *tmpURLStr = [[self URL]absoluteString];

        NSRange range_srch_book = [tmpURLStr rangeOfString:@"v1/books"];

        if (range_srch_book.location != NSNotFound) 
            [delegate performSelectorOnMainThread:@selector(parseDidComplete_srch_book) withObject:nil waitUntilDone:YES];

        [pool release];
    } 


    - (void) parseXMLFile: (NSURL *) url
    {   
        NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
        [self setURL: url];
        NSThread* myThread = [[NSThread alloc] initWithTarget:self
                                                     selector:@selector(run:)


object: nil];
    [retrievedArray removeAllObjects];
    [myThread start];
    [pool release];
}

There seems to be some synchronization issues if the user types very quickly (It seems to be working fine if the user types slowly)....So there are 2 views in which the content of an object in this shared array item is displayed; List and Detail. If user types fast and clicks on A in List view, he is shown B in detail view...That is the main issue.

I have tried literally all the solutions I could think of, but am still unable to fix the issue.

EDITING FOR SYNC ISSUE EXAMPLE: In the list view, if there are 3 items shown, say Item1, Item2 and Item3 and if user clicks on Item2, he is shown Item3 in detail view (i.e. to say not the correct details)

Below is the code that gets executed when an item in list view is clicked;

- (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath {
    // Navigation logic -- create and push a new view controller

    if(bookdetailCustom == nil)
        bookdetailCustom = [[BookDetailCustom alloc] initWithNibName:@"BookDetailCustom" bundle:[NSBundle mainBundle]];

    //aBook = [retrievedArray objectAtIndex:indexPath.row];

    bookdetailCustom.selectedIndex = indexPath.row;

    [self.navigationController pushViewController:bookdetailCustom animated:YES];
    [bookdetailCustom release];
    bookdetailCustom = nil;
}

Here is how searchTabkleView looks like

- (void) searchTableView {
    NSString *searchText = searchBar.text;
    NSMutableArray *searchArray = [[NSMutableArray alloc] init];

    for (int i=0;i<[retrievedArray count];i++)
    {
        Stock *aBookTemp = [retrievedArray objectAtIndex:i];
        NSString *temp = [aBookTemp valueForKey:@"BookName"];
        [searchArray addObject:temp];
    }

    for (NSString *sTemp in searchArray)
    {
        NSRange titleResultsRange = [sTemp rangeOfString:searchText options:NSCaseInsensitiveSearch];

        if (titleResultsRange.length > 0)
            [copyListOfItems addObject:sTemp];
    }

    [searchArray release];
    searchArray = nil;
}

Please suggest some suitable fixes.

Reprobate

Answer 1

From what you've posted, every retrievedArray is pointing at the same NSMutableArray object. So there aren't any separate arrays to keep synchronized, it's all the same array.

However, NSMutableArray is not thread safe; things may blow up if one thread is changing it while another is reading it. Simply changing the property from nonatomic to atomic is insufficient, because that only covers fetching the array object itself and not the subsequent method calls to access elements inside the array. I don't think that is causing your main issue, though, and the fix for that should obviate the thread safety issue.

I guess the sequence of events is something like this:

1. The List view is displaying a set of results that includes A at index N.
2. The user types something. The XML parser starts to update the shared array incrementally. The List view isn't updated yet.
3. The user touches the item at index N in the List view. The list view instructs the Detail view to display the item at index N.
4. The Detail view extracts the item at index N from the shared array, but due to the update started in step 2 index N now contains B. Which the Detail view displays.
5. At some point the XML parse completes, and now List is updated.

It should also be possible, if the load and parse from the web service is slow enough, for step 4 to simply crash with an NSRangeException.

One solution would be for each item in the List to hold the actual result object and pass that to the Detail view rather than just the index. You might be able to completely get rid of the shared array in this case, if List and Detail are the only consumers or if any other consumers can be changed to take objects instead of indices in the same way. Another would be for the parser to accumulate the results into a private array, and update the shared array all at once just before signaling the List view to update itself; there is still a slight possibility for a race in the time between the update on the background thread and the method invocation on the main thread, but the window is probably quite a bit smaller.

Or I could be completely wrong in my guess on how the update works, in which case you should provide more details.

Farina

Hey Anomie...It was very difficult for me to explain the issue over here. But you seemed to have understood the problem VERY ACCURATELY... Now coming to the solution, I would want to move towards the 2nd approach suggested by you. "Another would be for the parser to accumulate the results into a private array, and update the shared array all at once just before signaling the List view to update itself" Would it be possible for you to provide a pseudo code of what you are trying to say. I can implement the same in my app and see if it works. – Reprobate

But yes,as I said, since the issue occurs only when the user types very fast, it seems to have something to do with time taken to update the array in 2 places. Thanks again for all your help on this. I have given my best to fix the issue without any luck and am now really desperate to fix the issue. – Reprobate

In your NSXMLParserDelegate methods you must be adding objects into retrievedArray. Instead, add a temporaryArray field alongside the retrievedArray field, set that to a new NSMutableArray just before calling [parser parse], add the results to temporaryArray in the delegate methods, and then after [parser parse] returns call [retrievedArray replaceObjectsInRange:NSMakeRange(0,retrievedArray.count) withObjectsFromArray:temporaryArray]. – Farina

Or if you go to a little extra work to signal everything to reload retrievedArray from [[Shared sharedManager] books] (or just access that directly instead of copying the pointer to retrievedArray all over the place), you could just use [[Shared sharedManager] setBooks:temporaryArray]. – Farina

Hi Anomie..Thanks for your help..I am trying to understand the approach given by you and implementing the same in my app..I am a bit unclear with the 2nd approach (signal everything to reload retrievedArray). Could you please elaborate a bit on that... I'll also get back with my findings for the 1st approach as soon as I can. Thanks again. – Reprobate

Hi Anomie...I just tried implementing the 1st approach provided by you. I only edited the XMLParser class (i.e. replaced retrievedArray with temporaryArray in the delegate methods Also once the parse is complete, I have used [retrievedArray replaceObjectsInRange:NSMakeRange(0,retrievedArray.count) withObjectsFromArray:temporaryArray]; The values are getting filled in temporaryArray..However the sync issues still occur between the list and detail view. i.e. if I click on Item1, the details show for Item3. Please suggest. – Reprobate

Elaborating on the second approach: if you use [[Shared sharedManager] setBooks:temporaryArray], that will completely replace the array you get back from [[Shared sharedManager] books]. But anything that has already done that and saved the result to retrievedArray will not notice the change, they would have to do retrievedArray = [[Shared sharedManager] books]; again to update it. Or you could get rid of the retrievedArray as an ivar completely, and instead assign a local variable directly from [[Shared sharedManager] books] at the top of every method that needs the books list. – Farina

Hmm... Perhaps you should edit the question to include the handler for clicking on Item1. – Farina

ok..I edited the question to include the Item1,2,3 example...I'll also try and understand the 2nd approach and see if it works in my app..I'll get back to you once I have the findings. – Reprobate

I meant for you include the actual code that is executed when clicking on one of the items. – Farina

ok..I edited my question to include the select code. But still unable to fix the issue... – Reprobate

Please let me know in case I need to provide additional info. – Reprobate

Anomie: Did you get a chance to go through the updated code I added ? – Reprobate

I'm running out of ideas. What does the XML parser look like now after all the changes above, what does parseDidComplete_srch do on the list view controller, what does tableView:cellForRowAtIndexPath: look like, and what does whatever code in BookDetailCustom that actually loads the data look like? – Farina

I have edited my question to include searchTableView code as well... parseDidComplete_srch uses searchTableView cellForRowAtIndexPath uses copyListOfTems to display the indexpath.row object In BookDetailCustom, it uses index of retrievedArray to display in detail view. This info is passed to it from List view. – Reprobate

Do you think an almost duplicate copy array "copyListOfItems" can cause the issue. ? – Reprobate

I think I see your problem now. The rows in the List view correspond to the indexes in copyListOfItems, while the Detail view looks in retrievedArray. These arrays do not have the same content! You could do something like bookdetailCustom.selectedIndex = [retrievedArray indexOfObject:[copyListOfItems objectForIndex:indexPath.row]];, or you could just change BookDetailCustom to take the object instead of the index path. – Farina

I just tried bookdetailCustom.selectedIndex = [retrievedArray indexOfObject:[copyListOfItems objectForIndex:indexPath.row]]; But it is giving an error for objectForIndex: (not recognize/out of bounds) Also it shows duplicate items in the list view if I type very quickly in the search bar. Not sure what is the reason.... Anyways, I am almost sure the issue is because of these 2 arrays copylistofitems and retrievedArray... Do you want me to open a new question where in I'll highlight the 2 arrays thing specifically? – Reprobate

Bah, that wouldn't have worked anyway, I overlooked the fact that retirevedArray contains objects while copyListOfItems contains just the book name string. As I said earlier, the best thing to do would probably be to have each table cell hold on to the actual Stock object and have BookDetailCustom take a Stock object instead of an index. – Farina

ok..I updated the code in list view to stockdetailCustom.aBook = [retrievedArray objectAtIndex:indexPath.row]; and in detail view to NSString *tempOffName = aBook.OfficialName; It shows the same name in detail as the clicked LIST item But I am getting the following error after some time; [NSMutableArray objectAtIndex:]: index 20 beyond bounds [0 .. 19]' Not sure if it is related to this...But just to add, my list view also shows duplicate items..it should only show around 10 items.. – Reprobate

So to summarize I want the following things to happen; 1. Only show around 10 unique items in list view 2. Show clicked item in detail view 3. Do not show any duplicates (The API does not return any duplicates..it is because the array does not seem to be getting cleared..even though i have used removeAllObjects) 4. It should work even if the user types very fast. – Reprobate

When you change the list, are you calling reloadData on the table view? If you don't, it's possible that could cause both the duplicates and the eventual exception. Otherwise, under the Run menu in XCode is "Stop on Objective-C Exceptions"; enable that and it will stop when that exception occurs, and then you can use the normal debugger controls to find out exactly where in your program the out of bounds access is occurring at. – Farina

Yes, I am calling reloadData on my table. So as of now, the status is 1) List does not reload fast if user types quickly in the search bar. 20 However, the list/detail sync is proper i.e. if ABC is clicked in list view, it will show ABC's detail in detail view. (even though user might have quickly typed ABD, which did not refresh quickly/correctly) Anyways I'll accept the answer for now and open a new question describing the above status. – Reprobate

Thanks for all your efforts on this one..I'll open a new one in some time. – Reprobate

Answer 2

I originally suggested that you remove the nonatomic keyword from your property declaration. Atomic is the default (there is no atomic setting, omitting nonatomic is sufficient) - which will handle thread safety for you by wrapping the synthesized setter in a @synchronize block.

Unfortunately, many people have learned just to put nonatomic all over their code without really understanding it. I've always thought that this comes from copy/paste out of Apple sample code -- they use it often for UI-related stuff -- remember that UIKit isn't thread safe.

Anomie has indicated in his/her answer that this isn't it -- likely -- because you're mutating a mutable array from different threads. That sounds like the right answer to me - I'd delete my answer but I will leave it here as I think my comments are worth something (yet not 100% relevant to your problem).

Gibson

Thanks for your reply...I removed nonatomic from the Shared.h file @property (retain) NSMutableArray *books; But still the problem occurs... – Reprobate

Having an atomic getter and setter means that getting and setting the array will be atomic, but accessing the contents of the array still won't be. You are right that it should be atomic, but he also needs to use @synchronize around the code that modifies/reads the array. It would be simpler and more efficient to use @Anomie's solution of using a separate array during the update, in which case atomic setters would be sufficient. – Stave

If the Shared object had @synchronized methods to manipulate the books array, the concurrent access to the contents of the array would be solved. – Unintelligent

Answer 3

I understand that you've invested a lot of time and effort in fixing this and Anomie's solution is optimal for this. But maybe a different kind of approach might be easier to implement.

For example, you could have the parser process the data and feed it to a Core Data store. The list would be, in turn, fed by an NSFetchedResultsController. The controller automatically takes care of the table contents and any syncing that needs to be done.

It is worth trying and I hope it helps.

Boeschen

Answer 4

Try using an NSRecursiveLock in the accessors for the array.

See the NSRecursiveLock documentation. From the overview:

NSRecursiveLock defines a lock that may be acquired multiple times by the same thread without causing a deadlock, a situation where a thread is permanently blocked waiting for itself to relinquish a lock. While the locking thread has one or more locks, all other threads are prevented from accessing the code protected by the lock.

The CoreVideo sample code has examples of its proper use.

Crossland

I have never heard or used NSRecursiveLock before. Could you please elaborate more on how to implement the same in my app. – Reprobate

Why recursive lock? Shouldn't a normal lock be sufficient? – Stave

Answer 5

The problem is that retrievedArray is being referenced by two threads. Remove all references to retrievedArray from your XML parsing code and only change it on the main thread.

Here's the process:

1. Change parseXMLFile: to create a new array: parsedArray = [NSMutableArray array]
2. Change parser:didEndElement: to append to this new array: [parsedArray addObject:aBook]

3. In parser:didEndDocument: pass your new array off to the main thread:

   [delegate performSelectorOnMainThread: @selector(updateRetrievedArray:)
                              withObject: parsedArray
                           waitUntilDone: NO];
   

4. updateRetrievedArray: running on the main thread would be the code responsible for updating retrievedArray -- this way only one thread changes this object:

   - (void) updateRetrievedArray: (NSArray *)parsedArray {
       [retrievedArray setArray:parsedArray];
       [self parseDidComplete_srch_book]; // Be sure to call [tableView reloadData]
   }
   

Dijon

retrievedArray is a shared array and is updated as follows in XMLParser. First all objects are removed every time parsing is done; - (void) parseXMLFile: (NSURL *) url { [retrievedArray removeAllObjects]; .... } Then in parser:didEndElement( f([elementName isEqualToString:@"BookDetails"]) [retrievedArray addObject:aBook]; } please let me know in case you need any additional details. – Reprobate

I updated my answer based on your comment. Give it a try and let me know if it doesn't solve your issue. – Dijon

Yes sure..I'll try and implement the same in my app. I'll get back with the findings. – Reprobate

Just 1 quick question...For step 3 ...parser:didEndDocument: do you mean parser:didEndElement: Please confirm – Reprobate

I did try the code above. But for some reasons it is not working fine. I am unable to parse correctly. – Reprobate

No, #3 should be when you finish parsing the document -- if that's when you've been updating the table. Every time you parse an element, you add it to your parsedArray. When you finish parsing the document, you send the parsedArray to the main thread to update retrievedArray. But if your code isn't parsing correctly then that's a separate issue altogether... – Dijon