search

mentrena / SyncKit

Automatic CloudKit synchronization
https://mentrena.github.io/SyncKit/
MIT License
506 stars 57 forks source link

Forgotten local changes #114

Closed BlixLT closed 4 years ago

BlixLT commented 4 years ago

It looks like there is a bug in SyncKit that might cause missing changes (changes made locally not being registered (actually registered, but then forgotten) by SyncKit and therefore later not being uploaded to CloudKit). Firstly, I have a question - is there a reason why entity's updatedDate is being set in the targetContextDidSave method, but changedKeysArray is being updated in the targetContextWillSave? Shouldn't updatedDate also be set in the targetContextWillSave or is there a reason for not doing that?

The problem is that targetContextDidSave and targetContextWillSave notifications are asynchronous as is modifyRecordsOperation's modifyRecordsCompletionHandler.

So, after some testing I noticed, that it is possible such flow: You make a change to some NSManagedObject, SyncKit (CloudKitSynchronizer) registers that change and calls CKModifyRecordsOperation with a CKRecord for that object in the uploadRecords method. Lets say while Synchronizer waits for that operation's modifyRecordsCompletionBlock a new change for that object is being made -> CloudKitSynchronizer's adapter registers that change and sets changedKeys in the targetContextWillSave method, but in some cases modifyRecordsCompletionBlock is being called before targetContextDidSave notification and therefore updatedDate is not being updated yet. CoreDataAdapter compares CKRecord's CoreDataAdapter.timestampKey with entity's updatedDate, sees that they are the same and resets changedKeys array to an empty array. Its not so easy to reproduce each time, but I think we can agree that it is possible theoretically. And I see it sometimes - so, rarely, but possible.

To summarize - it seems that updatedDate logic is not quite correct. I think updating it right after setting new changedKeysArray in the targetContextWillSave should fix that issue. Unless there is some important reason why updatedDate is now being set in the targetContextDidSave. But I fail to see a good reason for doing that.

mentrena commented 4 years ago

That sounds like a good solution, I've added a commit to the Pull Request to do that.