3Squared / SQKDataKit

Lightweight Core Data helper to reduce boilerplate code.
MIT License
19 stars 6 forks source link

`sqk_insertOrUpdate` uses `performBlockAndWait` but doesn't save #10

Closed blork closed 9 years ago

blork commented 10 years ago

Since this method leaves it up to the caller to save, you'd have to do something like this:

[Bookmark sqk_insertOrUpdate:responseObject
            uniqueModelKey:@"href"
           uniqueRemoteKey:@"href"
       propertySetterBlock:^(NSDictionary *dictionary, Bookmark *bookmark) {
       } privateContext:privateContext
                     error:nil];
[privateContext save:nil];

But then you are calling save: outside of performBlockAndWait: which is a threading violation. The solution is either:

lukestringer90 commented 10 years ago

I think this approach provides the simplest API, but has a problem:

save inside sqk_insertOrUpdate:

As you eluded to, I don't think this method should have responsibility of saving because you might want to save at a different time. This is particularly true if you are performing lots of core data interactions where having control of when the context is saved so that you can save in batches is useful. If sqk_insertOrUpdate: owns this responsibility then staggering saves is not possible.

So for me sqk_insertOrUpdate: should not save and we should document that it needs calling form within a performBlockAndWait: block. SQKDataImportOperation does this just so at least we are adhering to this policy, we just need to make it clear in the apple doc.

Can we find the information in the official Core Data documentation in regards to using performBlockAndWait: ? Would be useful to reference this in out own apple doc for the sqk_insertOrUpdate:.

blork commented 10 years ago

Everything you need to know is in the concurrency section of this page. I don't think it's necessary to add anything to the README unless we want to start reproducing whole parts of the SDK documentation. But, for clarity:

You use contexts using the queue-based concurrency types in conjunction with performBlock: and performBlockAndWait:. You group “standard” messages to send to the context within a block to pass to one of these methods. There are two exceptions:

Setter methods on queue-based managed object contexts are thread-safe. You can invoke these methods directly on any thread. If your code is executing on the main thread, you can invoke methods on the main queue style contexts directly instead of using the block based API. performBlock: and performBlockAndWait: ensure the block operations are executed on the queue specified for the context. The performBlock: method returns immediately and the context executes the block methods on its own thread. With the performBlockAndWait: method, the context still executes the block methods on its own thread, but the method doesn’t return until the block is executed.

It’s important to appreciate that blocks are executed as a distinct body of work. As soon as your block ends, anyone else can enqueue another block, undo changes, reset the context, and so on. Thus blocks may be quite large, and typically end by invoking save:.

lukestringer90 commented 10 years ago

@blork Will this addition to the documentation suffice?

blork commented 10 years ago

The performBlockAndWait in the method is redundant, then. Shouldn't cause a problem but it's unnecessary.