Ensure data consistency between ASDataController and its delegate while executing update transactions

[nguyenhuy]

My attempt to fix #619. The idea is to capture ASDataController's _completedNodes and use that cache for any external queries until a current update transaction finishes.

Let me know what you think on this diff. If we don't want to cater ASDataController to work with UITableView and UICollectionView specific behaviours, I can simply make a wrapper/subclass that sits between UITableView/UICollectionView and ASDataController. It can intercept transaction calls as well as external data queries, and ensures data consistency using the same technique here.

I tested this diff by abusing Kitten sample with a bunch of insertions, deletions and device rotations. Really appreciate if @eanagel, @ryanfitz, @Adlai-Holler and @smyrgl can help me to test as well.

I have another diff that tries to fix the issue by calling beginUpdates, edit command blocks and endUpdates in a single run loop. That diff is more complex and doesn't cover all the edge cases:

• It has to catch and queue all delegating blocks, then executes them on main thread.
• It doesn't handle the case when data is accessed externally after some edit commands finished, but before the (single) beginUpdates/endUpdates run loop is executed. It crashes because _completedNodes was modified and is different from what the table view/collection view expects.

[nguyenhuy]

P/S: Will add some automated tests once existing tests can be run on Travis reliably (i.e XCode 7 and asyncdisplaykit_node madness).

[appleguy]

Thanks @nguyenhuy !! Just FYI, I believe the build server is working; we can't use Xcode 7 on Travis, but the tests should still be running with 6.4.

[nguyenhuy]

But I naively updated my XCode to 7. Now tests fails locally :(

[appleguy]

:(. I just downloaded 6.4 from Apple's website, which makes the DMG available. Inconvenient, but after spending well over an hour failing to understand the build issue, I can't think of an alternative in the near term about the test issue on Xcode 7...

[nguyenhuy]

Yeah, I will try to look at that issue tomorrow. Will fall back to 6.4 if I have to. Thanks for the info.

[appleguy]

Move the ASDisplayNodeAssertMainThread() to this method.

I'm wondering if the existing accessor -completedNodes makes sense. I know it is API exposed on ASDataController, I think used by ASRangeController. Right now the names between these don't quite sit right with me, particularly because the external one is mutable, and also is not /actually/ externally exposed at all (at least the method name).

Could we just delete one of these methods, and rename externalCompletedNodes to completedNodes? Not sure if having the header show NSArray as the return type will work seamlessly if the implementation actually returns NSMutableArray.

[nguyenhuy]

Yeah, the main thing is -nodesAtIndexPaths: needs a mutable array (here). Maybe casting it is fine?

Not sure if having the header show NSArray as the return type will work seamlessly if the implementation actually returns NSMutableArray.

I either get a NSArray (and still need to cast), or a "duplicate declaration of method completedNodes" error :(

[appleguy]

Does this address the order of applying edits, mentioned in #684?

[nguyenhuy]

I don't think it does :(

[nguyenhuy]

@Adlai-Holler explains the problem clearly here:

For example, say you have 1 section with 7 items ABCDEFG. You want to do a batch update with (move 5 to 1), (insert X at 0), so you expect the end result to be XFABCDEG.
If you tell ASTableView about the changes in that order you'll end up with XAFBCDEG which is wrong. If you tell ASTableView about the changes in reverse order you'll end up with XEABCDFG

[nguyenhuy]

@appleguy Pushed a new commit that removes externalCompletedNodes getter. Let me know what you think about the cast here. Also, do you think I should remove main thread asserts in methods that call completedNodes getter and assume the assert is done in that getter? Does the runtime overhead justifies the assumption?

[smyrgl]

@nguyenhuy So the order of edits still matters here? You still expect this diff to cause a crash when the edits are not specifically ordered?

When I tested this diff in my app I still got crashes, this log snippet should give you an idea of what is going on. I purposefully disabled any kind of sorting of the updates (as you can probably see) and just applied them in the natural order they were returned from the FRC.

�[;�[fg85,85,85;2015-09-27 16:46:34:302 Disqus[83690:7373064] Data model did update
�[;�[fg85,85,85;2015-09-27 16:46:34:303 Disqus[83690:7373064] UPDATE - InsertedSections: 11 DeletedSections: 0 InsertItems: 1, ModifiedItems: 0, DeletedItems: 1, MovedItems: 0, MovedSections: 0
�[;�[fg85,85,85;2015-09-27 16:46:34:303 Disqus[83690:7373064] Beginning table view updates
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 2
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 3
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 4
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 6
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 12
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 13
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 15
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 16
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 19
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 21
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting section 22
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Inserting item  at index path <NSIndexPath: 0xc000000001400816> {length = 2, path = 8 - 10}
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Deleting item at index path []
�[;�[fg85,85,85;2015-09-27 16:46:34:304 Disqus[83690:7373064] Data source locking
�[;�[fg85,85,85;2015-09-27 16:46:34:305 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:305 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000216> {length = 2, path = 2 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:306 Disqus[83690:7373064] Number of rows in section: Optional(2)
�[;�[fg85,85,85;2015-09-27 16:46:34:306 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000316> {length = 2, path = 3 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:307 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000200316> {length = 2, path = 3 - 1}
�[;�[fg85,85,85;2015-09-27 16:46:34:309 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:309 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000416> {length = 2, path = 4 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:310 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:310 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000616> {length = 2, path = 6 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:310 Disqus[83690:7373064] Number of rows in section: Optional(7)
�[;�[fg85,85,85;2015-09-27 16:46:34:311 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000c16> {length = 2, path = 12 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:311 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000200c16> {length = 2, path = 12 - 1}
�[;�[fg85,85,85;2015-09-27 16:46:34:313 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000400c16> {length = 2, path = 12 - 2}
�[;�[fg85,85,85;2015-09-27 16:46:34:314 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000600c16> {length = 2, path = 12 - 3}
�[;�[fg85,85,85;2015-09-27 16:46:34:315 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000800c16> {length = 2, path = 12 - 4}
�[;�[fg85,85,85;2015-09-27 16:46:34:316 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000a00c16> {length = 2, path = 12 - 5}
�[;�[fg85,85,85;2015-09-27 16:46:34:317 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000c00c16> {length = 2, path = 12 - 6}
�[;�[fg85,85,85;2015-09-27 16:46:34:318 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:318 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000d16> {length = 2, path = 13 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:319 Disqus[83690:7373064] Number of rows in section: Optional(2)
�[;�[fg85,85,85;2015-09-27 16:46:34:319 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000000f16> {length = 2, path = 15 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:320 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000200f16> {length = 2, path = 15 - 1}
�[;�[fg85,85,85;2015-09-27 16:46:34:321 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:321 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000001016> {length = 2, path = 16 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:322 Disqus[83690:7373064] Number of rows in section: Optional(4)
�[;�[fg85,85,85;2015-09-27 16:46:34:322 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000001316> {length = 2, path = 19 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:322 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000201316> {length = 2, path = 19 - 1}
�[;�[fg85,85,85;2015-09-27 16:46:34:323 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000401316> {length = 2, path = 19 - 2}
�[;�[fg85,85,85;2015-09-27 16:46:34:324 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000601316> {length = 2, path = 19 - 3}
�[;�[fg85,85,85;2015-09-27 16:46:34:325 Disqus[83690:7373064] Number of rows in section: Optional(11)
�[;�[fg85,85,85;2015-09-27 16:46:34:325 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000001516> {length = 2, path = 21 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:326 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000201516> {length = 2, path = 21 - 1}
�[;�[fg85,85,85;2015-09-27 16:46:34:326 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000401516> {length = 2, path = 21 - 2}
�[;�[fg85,85,85;2015-09-27 16:46:34:327 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000601516> {length = 2, path = 21 - 3}
�[;�[fg85,85,85;2015-09-27 16:46:34:328 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000801516> {length = 2, path = 21 - 4}
�[;�[fg85,85,85;2015-09-27 16:46:34:329 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000a01516> {length = 2, path = 21 - 5}
�[;�[fg85,85,85;2015-09-27 16:46:34:330 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000c01516> {length = 2, path = 21 - 6}
�[;�[fg85,85,85;2015-09-27 16:46:34:331 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000e01516> {length = 2, path = 21 - 7}
�[;�[fg85,85,85;2015-09-27 16:46:34:332 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000001001516> {length = 2, path = 21 - 8}
�[;�[fg85,85,85;2015-09-27 16:46:34:332 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000001201516> {length = 2, path = 21 - 9}
�[;�[fg85,85,85;2015-09-27 16:46:34:333 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000001401516> {length = 2, path = 21 - 10}
�[;�[fg85,85,85;2015-09-27 16:46:34:334 Disqus[83690:7373064] Number of rows in section: Optional(1)
�[;�[fg85,85,85;2015-09-27 16:46:34:334 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000000001616> {length = 2, path = 22 - 0}
�[;�[fg85,85,85;2015-09-27 16:46:34:335 Disqus[83690:7373064] Data source unlocking
�[;�[fg85,85,85;2015-09-27 16:46:34:369 Disqus[83690:7373064] Data source locking
�[;�[fg85,85,85;2015-09-27 16:46:34:369 Disqus[83690:7373064] Rebuilding node at index path <NSIndexPath: 0xc000000001400816> {length = 2, path = 8 - 10}
�[;�[fg85,85,85;2015-09-27 16:46:34:370 Disqus[83690:7373064] Data source unlocking

EDIT: Oh and the crash occurred at line 93 of ASMultidimensionalArrayUtils. If you want more info or want me to try some other stuff just let me know.

[nguyenhuy]

@smyrgl well yeah, as mentioned in #684, I think wrong edit order is another separate problem and will be fixed later on. To test this PR, you have to make sorted edit batch and query nodes while the batch is being executed (calling nodeForRowAtIndexPath for example). But thanks for confirming that sorting is still needed and provide the log. It will be very useful for the other fix, and even more so if you post a sample project that crashes :)

[appleguy]

OK, this is a lot better, thanks @nguyenhuy! In it goes :). Later I may rename the new instance variable, although I haven't thought of a better name yet. It seems like it should reflect the fact that it is temporary / transient while committing a transaction of batch edits. It is not very important at all because it is entirely internal to the implementation.

I'm not sure who is going to attempt a fix to the ordering issue, but we should likely introduce that "transaction" concept for the batches.

[nguyenhuy]

Thanks, @appleguy. I'm not very proud of the name either, but it was the best I could come up with (other alternatives were backingStoreNodes and delegateStateNodes). So feel free to change it. May "shadowWhileBatchingNodes" sound better? Man, I really suck am really bad at naming things.

@Adlai-Holler said he can take the ordering issue.

[smyrgl]

@nguyenhuy I would be glad to slice off a sample project with one of my core view controllers (can't post the whole project itself for obvious reasons) but it should be plenty for you to test with. I will try to get this done tonight and I'll post a link in here.

[nguyenhuy]

@smyrgl That would be awesome. Please do!

[eanagel]

@nguyenhuy Just catching up after being off for a couple days. This looks like an elegant solution to this tricky issue. I agree that moving all batched updates into a single run loop is the best answer but it represents a significant refactor that is intimidating to say the least. Thanks so much for figuring this out!

[nguyenhuy]

@eanagel my pleasure. Please test it and report any problems found :)