-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation flaw #5
Comments
Isn’t this the same as with the synchronized data you should be saving on your own? When you fail to save the received changes, there’s a risk of losing them. I approach it in a way where I persist a representation of the pending changes alongside the synced data. Whenever a save succeeds, as indicated by the By the way, I’m only referring to the design of |
I agree: Talking about In other words: I wouldn’t rely on |
Do you mean the delegate callback when CKSynEngine fetched changes?
I have thought about this, but it is very easy to get it wrong: An unfortunate sequence of events could result in your representation of your pending changes removing changes that were triggered by other pending changes. There must be a way to associate the CKSyncEngine.Event.SentRecordZoneChanges callback with the changes you want to remove from your own persistence layer, but I don't see any way of doing this.
|
But CKSyncEngine already manages state for this very reason - and we need to persist that state. I think this is a great design, but the way it is implemented makes it very hard to implement correct behaviour. I know it's just a sample app, but I think it would be much better to use the sample app to show how to use CKSynEngine for these cases. I'm pretty sure most developers just assume that when they call .add(pendingRecordZoneChanges: pendingSaves) the sync engine will do the rest - but it doesn't (for edge cases).
This is exactly what confuses me. There is an api for this, but it does not handle these edge cases. This type of crash is definitely very unlikely, but if it happens the sync is broken. Also, I don't see an easy way to associate the delegate callback with the changes you've persisted - it's tricky to get that right too. A simple flag won't work in certain event sequences. |
I would be cautious to assume anything about the Anyway, I suspect that there are multiple interpretations of how to use |
Exactly. If you fail to integrate these into your persistence layer, you’ll lose them.
I don’t really understand what you mean by this. Can you elaborate?
Currently, in my usage of However, after thinking about it for a moment, there indeed might be a problematic sequence:
In this case, the second change made in step 5 would be ignored. I guess the safest way would probably be to store pending changes into a secondary bucket after receiving the sync engine starts sending (step 3). Then, when it finishes (step 8), you’d move them to the primary bucket and signal changes to the By the way, I also wanted to point out that you don’t have to use @jmjauer Is this the sequence you had in mind? |
I’d say there’s one step missing in your sequence of events: After step 5, you’d have to signal the sync engine again that there’s a change to sync. It would then decide on when to do that by itself. This also means your persistence layer’s representation of a pending change would need to be rich enough to handle the case that there might be multiple pending sync operations for one single record. So not just a boolean but something kind of like a semaphore that counts up for pending changes and down for completed syncs. |
Yes, this is another problem I don't know how to fix. Probably delete the sync state and restart the sync.
Can you explain that a little bit more?
Yes, indeed |
I also thought about this, but I think there are no guarantees for the number of callbacks. And if the number of callbacks is less than the number of addPending calls, this will not work. |
Great discussion here! You're right that there's a potential problem if the process crashes in between a call to That said, this is definitely a real issue. We can track this internally and figure out a resolution, but it would hold more weight if we could get some of your opinions and perspectives as well. The more people we see who care about the issue, the more urgently we can prioritize it. Would you mind submitting a feedback about this? To address some specific points:
Associating the data in
Fortunately, the fetching side of this doesn't have the same issue.
If you're using
Since you told the sync engine that you have a new pending save in step 3 while the previous change was in flight, it will keep that pending change even after step 4. If you're using
Hopefully that clears some stuff up. Again, this is a great discussion, and we're happy to keep it going until we get to the bottom of everything. Also, sorry for the delay on responding to this! |
Hey Tim, thanks for joining the discussion!
I will submit feedback on this issue. I think a simple solution would be to add async versions of
Maybe I'm missing something, but wouldn't it be relatively easy for
This is really interesting - can you explain how CKSyncEngine tracks this in-flight status for each record when using Overall, I think |
❤️ Thanks!
Something like that could potentially work. Or we could even let you put some sort of arbitrary object on
The general flow looks like this, although the exact implementation is subject to change:
The key piece here is steps 3 and 4. If you're using You could also achieve the same result by tracking some sort of explicit "version" each object. You'd have to know the current version locally as well as the last known version on the server. If the server version is less than the local version, then you know you still have changes to send to the server. This is less "in-flight" tracking and more just tracking whether or not you've saved everything to the server. This version could be as simple as an integer or as complex as a vector clock.
I agree it would be interesting to see a sample project that uses |
Your welcome! The feedback ids are FB13469853 and FB13469895.
I will try to implement this for my use case and get back to you with my findings and maybe suggestions for improvement :) |
I am currently in the process of implementing sync using
|
Hi there,
I think there is an implementation error in the sample project - more specifically, a flaw in the design of CKSyncEngine.
In the SyncedDatabase file, line 350:
self.syncEngine.state.add(pendingRecordZoneChanges: pendingSaves)
This tells the syncEngine what to do when the time is right. Since this piece of information is not persisted in this call (I think its persisted in .stateUpdate(let event)), there is a chance that this information is lost if the app crashes between .add(pendingRecordZoneChanges: pendingSaves) and .stateUpdate(let event).
I know this is just a sample project, but I really think this is a flaw in the API design of CKSyncEngine since I do not see an easy way to guarantee correct behavior. A simple fix would be to make .add(pendingRecordZoneChanges: pendingSaves) (and similar methods) async and only complete after .stateUpdate(let event) was called.
What do you think?
The text was updated successfully, but these errors were encountered: