Concurrency thread limitations in CoreDataStackFactory not necessary

[wiedem]

The current implementations in the createStackInBackground and createStack methods of the CoreDataStackFactory have some technically unnecessary limitations regarding the calling thread (precondition statements).

Creating queue based contexts with the NSPrivateQueueConcurrencyType and NSMainQueueConcurrencyType on any thread (including the main thread) is absolutely legit and valid.

As described in the NSManagedObjectContext Class Reference only contexts using the deprecated confinement pattern must be created and used from a single thread. I.e. those contexts are no longer valid if the thread to which the context was bound during the init phase ends.
Queue based contexts on the other hand must not be used with anything else but the performBlock and performBlockAndWait which automatically ensures that the operations are run in a correct context (thread).

That being said:

• the precondition in createStack of CoreDataStackFactory is unnecessary. Calling this method asynchronously would be totally fine, both backgroundContext and the mainContext would work as expected.
• the precondition in createStackInBackground of CoreDataStackFactory is also unnecessary.
• CoreDataFunctions and the resetStack have the same limitation.

It does however make sense for some of the mentioned methods to be called asynchronously (or to be called on the main queue) even though there's no technical requirement to do so.
If the mentioned limitations are intended because it helps the UI to stay responsive then this should be at least documented. Otherwise it would be a good thing to give developers the freedom to use those methods on any queue they want for whatever reason there may be.

[jessesquires]

Thanks @wiedem ! 🎉 😄

Sounds like we can remove createStackInBackground, remove the preconditions, and update docs for createStack() to specify that it should be called on a background thread.

Would you like to submit a PR?

[wiedem]

Thanks @wiedem ! 🎉 😄

You're welcome 😎

Sounds like we can remove createStackInBackground, remove the preconditions, and update docs for createStack() to specify that it should be called on a background thread.

Well, I personally wouldn't remove the async method but rather make it the default and remove the synchronous one. Adding persistent stores to the psc asynchronously is a good practice.
I still would remove the precondition check though. Maybe I would even remove the whole queue parameter and always use a background queue for adding the persistent store. I'm not sure if it's an actual use case wanting to specify a custom queue for adding the store.

Besides what I just mentioned: have you ever considered providing a convenient method to listen for store events on the CoreDataStack? This listener closure would be called by the NSPersistentStoreCoordinatorStoresDidChangeNotification event whenever a store becomes available or is removed.
I usually listen to this exact event in my view controllers to update my views whenever a store is added or removed. The first event of this type will always be fired after the Core Data stack init is complete. Which could make it obsolete to use a separate completion callback for the init phase.

Would you like to submit a PR?

I would if I would be 100% sure how I would want to exactly change the mentioned methods, besides simply removing the preconditionstatements of course 😅

[jessesquires]

Ah good points. Only having createStack() would impose a lot of background queue boilerplate on clients.

I'll think about this a bit more and push some fixes! 😄

[jessesquires]

hey @wiedem - check the commit above for my solution here. 😄 let me know what you think!

[wiedem]

hey @wiedem - check the commit above for my solution here. 😄 let me know what you think!

The implementation seems fine to me 😉
I had sth. different in mind though, I will try to send a PR during the next days.

[wiedem]

@jessesquires I found some time to change the method, you can take a look here

What this change does is it synchronously creates the store coordinator and the mocs because this shouldn't be time critical tasks. The persistent store is on the other hand is added asynchronously because this is the potentially time consuming task that shouldn't block the main thread.
Therefore the method returns a CoreDataStack without persistent stores, which can be used for any read operations (which return empty results) but not for write operations.

The completion callback will be called once the store has been added, that's when the UI should refresh its content and should make write operations possible.

Let me know if you want me to create a PR. I haven't changed the corresponding tests yet.

[jessesquires]

Ah, that's interesting.

I think this is a bit too confusing for clients though — having a completion block and a return. I think there should be 1 way to get the stack and have it be fully ready to use. 😄

[wiedem]

You could of course simply remove the return value. As said, the advantage of the solution you see in my changes is that you can fully initialize your UI before your persistent stores are loaded since a valid CoreDataStack is already available. Once the stores are loaded / migrated your UI just needs to load the data to display.
Whereas if you wait for a full CoreDataStack to be available you cannot prepare your NSFetchedResultsController etc. until that is done. My solution is pretty close to what Apple describes in their Core Data Programming Guide.

But I understand if you want to do it your way.