gistfile1.md

Hey Mattt, Jerry here, we met at WWDC, and then again at Heroku. I thought I'd ping you and let you know some of the things I've found with AFIncrementalStore. In short, we've had some painful instability as our app has grown. We've seen crashes like EXC_BAD_ACCESS 0x13 (I made that specific number up, but you get the point) and random Unrecognized Selector exceptions in code that very obviously isn't sending incorrect messages -- all the hallmarks of memory corruption. I was more and more confident that we had some threading issues, and they were jeopardizing the success of this project. Maybe it was too early to use AFIS in a production app, but, why not live dangerously!?

Anyhow, I don't write to you with only doom and gloom -- after chaining myself to my laptop for 5 days or so, I believe I've come out on top. I've refactored the shit out of AFIS, namely reorganizing to ensure thread safety. I may try to submit a pull request or 12, but it's going to be a bit heavy, and I thought it might be useful to give you a heads up on the main issues I addressed (and spent a stupid number of hours exploring).

Managed Objects are not thread safe - This makes sense, but then consider that MOs belong to their context's and suddenly it stands to reason that they need to follow the same rules as the context with regard to threads. MOC operations with a private queue type are basically off limits outside of a performBlock: or performBlockAndWait:, and the same is true of their objects. There's more info on this in the 2012 Core Data best practices video. There's also good info on it in this SO thread (http://stackoverflow.com/questions/8637921/core-datas-nsprivatequeueconcurrencytype-and-sharing-objects-between-threads).

This means this is not kosher -- backingObject should be off limits outside the block (I think the WWDC session says you can retain them, but can't observe them or interact with them):

- (BOOL)insertOrUpdateObjectsFromRepresentations:(id)representationOrArrayOfRepresentations
                                        ofEntity:(NSEntityDescription *)entity
                                    fromResponse:(NSHTTPURLResponse *)response
                                     withContext:(NSManagedObjectContext *)context
                                           error:(NSError *__autoreleasing *)error
                                 completionBlock:(void (^)(NSArray *managedObjects, NSArray *backingObjects))completionBlock
{
	...
        [backingContext performBlockAndWait:^{
			//bacingObject assigned in here
        }];
        [backingObject setValue:resourceIdentifier forKey:kAFIncrementalStoreResourceIdentifierAttributeName];
        [backingObject setValue:lastModified forKey:kAFIncrementalStoreLastModifiedAttributeName];
        [backingObject setValuesForKeysWithDictionary:attributes];
	...
}

Nesting performBlock: is dangerous - I haven't seen deadlocks as significantly as in the past, but they happen, and holy shit is this easy to accidentally do. The worst part is, it's more or less impossible to identify deadlocks in the wild. I think it comes down to this, never lock back upstream. I don't have a great example (too many drinks), maybe this one:

- (id)executeFetchRequest:(NSFetchRequest *)fetchRequest
              withContext:(NSManagedObjectContext *)context
                    error:(NSError *__autoreleasing *)error
{
            [context performBlockAndWait:^{
				// stuff
                [childContext performBlockAndWait:^{
					// stuff
                        [backingContext performBlockAndWait:^{
							// stuff
                        }];
                        [context performBlockAndWait:^{
							// stuff
                        }];
                    }];
                }];
            }];
}

The WWDC talk discusses parents locking upon children, it's a great way to deadlock. If you want to see for yourself, try executing something like this rapidly in succession: [a performBlockAndWait:^{ [b performBlockAndWait:^{ [a performBlockAndWait:^{ /*fuck*/ }] }] }]; Promise, you'll deadlock.

General thread safety - Turns out (and I don't intend this to sound dickish), using the performBlock methods doesn't solve the issue of general thread safety, despite how magical they seem (seriously, I thought they were magical). Especially with performBlock:, it's likely that all the encapsulated code is running on a thread other than the main thread. This means that basic threading requirements end up in play, and it's super important to be mindful of any memory mutations that may take place. For example, objectIDForBackingObjectForEntity:withResourceIdentifier:(NSString *)resourceIdentifier, it mutates _backingObjectIDByObjectID. In newValuesForObjectWithID:withContext:error: it's called from a performBlock: call. This means that _backingObjectIDByObjectID is being read from and written to from a variety of threads -- obviously bad news, at least without some barriers and shit.

This is a lot of words, especially considering the feedback is unsolicited. Possibly, I'm a huge asshole for sending such a wordy email instead of taking the time to just submit some pull requests, but I've also been celebrating after 5 days of being chained to my computer, and now i'm a little drunk, and apparently don't care.\

I suspect it will be super annoying to check diffs this way, but all my changes are currently visible in a branch I have in my repo (https://github.com/Spaceman-Labs/AFIncrementalStore/blob/gs_threading/AFIncrementalStore/AFIncrementalStore.m). Take a look, if it's interesting, ping me -- email, twitter, whatever works.

Thanks!