On Mi, 2011-12-21 at 17:17 +0100, Lukas Zeller wrote: I've finally looked at this again, using the upstream gitorious repo's master branch. That has your "DB_Conflict (409): added missing implementation in binfileimplds.cpp" fix. The sequence of events now is: * dumb server has one item with UID=foo * smart client has an updated version of that same item, created there independently * client and server sync: * server stores two copies, then sends client the old copy (ID 0) as "new item" * client detects duplicate, resolves this internally by keeping its own copy and returning 201 to the server (instead of 409 as before); the <Map> sent to the server maps the server's item 0 to the same LUID also sent to the server as "new" from the client (stored there as item 1) * the server stores a UID mapping where its own old item 0 maps to the existing item foo on the client, and its new item 1 maps to no item on the client * client and server are now out of sync: the server still has old and new copy of the item, the client only the new one * client and server sync again: * client sends its copy as "updated" * server updates its old copy * client and server are still out of sync: one item too many on the server I'm not sure how to fix this. When processing the <Map>, the server must have detected that the same client ID was used twice. What it logs at the moment is this: [2012-01-30 18:01:19.775] Mapping remoteID='foo' to localID='0' [2012-01-30 18:01:19.775] file_calendar: testState=TRUE - expected state>='sync_mode_stable', found state=='sync_gen_done' [2012-01-30 18:01:19.775] ModifyMap called: aEntryType=normal, aLocalID='0, aRemoteid='foo', aMapflags=0x0, aDelete=0 [2012-01-30 18:01:19.775] - found entry by entrytype/localID='0' - remoteid='', mapflags=0x0, changed=0, deleted=0, added=0, markforresume=0, savedmark=1 [2012-01-30 18:01:19.775] - matching entry found - re-activating deleted and/or updating contents if needed [2012-01-30 18:01:19.775] - cleanup: removing same remoteID from other entry with localid='1', mapflags=0x0, changed=0, deleted=0, added=0, markforresume=0, savedmark=0 [2012-01-30 18:01:19.775] Map entry updated: LocalID='0', RemoteID='foo' Should it have removed one item while processing the <Map>? IMHO it can't, because it doesn't have enough information to determine which of its two items are up-to-date. It only knows that the client must have merged two items, but not yet which one won. In the second session, the server gets an updated item with client ID "foo". Now it could update one copy and delete the other, but because it only kept a link between its item 0 and foo, it only does the update part. Would something break if the server allowed the same client item to map to multiple items on the server and then did a remove in its database when receiving an update? I suspect that the client is simply doing something not expected by typical SyncML servers. Here's what other SyncML servers do: * Memotoo: doesn't send UID in its item data, thus the client does not detect the duplicates * Funambol: doesn't let me run a sync at the moment, need to test later -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.

On Mon, 2012-02-06 at 21:29 +0100, Patrick Ohly wrote: The result is in the "internal-sync" branch in the meego.gitorious.org repo of libsynthesis. It's called "internal-sync" because I am working on an extension of the SyncML protocol that is only understand when SyncEvolution talks to SyncEvolution (either in the SyncEvolution local sync mode - see the http://syncevolution.org/blogs/pohly/2012/fosdem-2012 talk for a diagram illustrating that) or in SyncEvolution client to SyncEvolution server mode. Instead of repeating myself, let me quote the commit messages of the relevant commits on that branch below. There are a few more commits in that branch. Lukas, what do you think? This is not done yet, but if possible I'd like to get feedback before going down an entirely wrong path. Some tests for this new code are in SyncEvolution (not pushed yet) and pass as expected. I'll keep working on this, in particular the "temporary problem" part and "add more data in second cycle" cases. commit 37910c119381eb9e8d64a3183b4f29f2fe5b246f Author: Patrick Ohly <patrick.ohly(a)intel.com&gt; Date: Tue Feb 7 10:56:52 2012 +0100 DB_Conflict (409): different implementation in binfileimplds.cpp The previous approach to handling a 409 status for an <Add> (= item conflicts with existing item) in the binfile client was to update that existing item and then tell the server that its item maps to an existing client item. This leads to the situation where the server has two client IDs mapping to the same item on the server and one server item with no corresponding client item. Servers get confused by this. For example, the Synthesis engine itself then sends a <Delete> with empty IDs to the client in the next sync. It also has the disadvantage that the client cannot ask the server to delete the redundant item, because its <Delete> requests must include a client ID, which the redundant item doesn't have. Furthermore, the server was sent a redundant <Update> in the case that it already had an item that was in sync in the client (= item didn't have to be written in local storage). Therefore this commit implements a different approach: - the new server item which triggers the 409 is used to update the existing item, as before - if and only if the local item gets modified, it will be sent as update for the older server item already mapped to it (there must be such an item, because the client must have told the server about that local item, and that server item is now out-dated) - the new server item is mapped to a fake client ID which is marked as deleted; in the next sync, that mapping is used to delete the new server item Once a second sync is done, client and server are in sync again, with the latest data as determined by the client stored in the servers older item and the newer item deleted. Generating a fake client ID is a bit hacky at the moment. The code for numeric IDs is entirely untested, the code for string IDs doesn't check for (unlikely?!) collisions. commit 6504ae35543efe860feeda4ce498ec9824a74d3d Author: Patrick Ohly <patrick.ohly(a)intel.com&gt; Date: Tue Feb 7 15:46:29 2012 +0100 SyncML extensions: multiple cycles in the same session SyncML limits the data exchange to "client sends changes, server sends changes back". If the client detects that it has further changes for the server while already in the second phase, it will have to wait for the next SyncML session before it can send these changes. This leads to a bad user experience, because the user expectation is that client and server are in sync after a sync session. Examples when this becomes a problem: - 409 conflict detected in binfile client (tested with SyncEvolution Client::Sync::eds_event::testAddBothSides[Refresh] and a SyncEvolution server with file backend which doesn't detect duplicates based on UID) - temporary error prevents applying a change, needs to be retried (untested at the moment) - PBAB use case: advanced client does a sync with a subset of the data (vCard without PHOTO, while retrieving PHOTO data in the background), then sends updates with more data (modified/new PHOTOs) later on (also untested) This commit introduces a new mode where SyncML client and server allow multiple read/write cycles inside the same session. This extends the SyncML 1.2 standard in the following way: - in refresh-from-client mode, client and server always enter the map phase although the standard says that it can and must be skipped; the Synthesis engine already supported that non-standard behavior via the fCompleteFromClientOnly flag (aka <completefromclientonly> in the config and remote rules) - the client, if it has more changes for the server and hasn't encountered problem, finishes the current cycle internally and then sends a new <Alert> in the next message to start the next cycle, instead of ending the map phase and package with a <Final/> tag - the server starts at the beginning of a new SyncML session when it gets that <Alert>, instead of rejecting it as a protocol violation - the first sync mode is done as set by the app; any following one falls back to incremental mode: - slow sync -> two-way sync or one-way if data direction was limited - refresh sync -> one-way sync in same direction - one-way sync -> do it again To simplify testing, this new functionality is currently limited to sync sessions with only a single active data store. The new mode has to be enabled *on both sides* with the <completefromclientonly> config option. When only enabled on the client side, the unexpected <Alert> will be rejected by the serer. When only enabled on the server side, the client might be surprised by the non-standard map phase behavior. SyncEvolution enables this new mode unconditionally with a remote rule when talking to itself. Ideally this should be replaced with a DevInf extension, both for "SyncML engine supports the mode" and "data store supports restarting". Data stores must be able to deal with repeated cycles in the same session. Typically this consists of reseting some state so that running another sync cycle doesn't reuse obsolete information. The binfile DS supports this now, but the underlying data store in the app might not be ready for it yet. Currently the engine does not check this. The binfile client automatically recognizes the 409 conflict case and enters a new sync cycle to get the server in sync with the client. In addition, any client can be told to restart the sync by setting the new "restartsync" session variable (PBAP use case). This has to happen before the client considers the session completed, i.e. before EndDataWrite() is called. It would be better to do this with a per-datastore flag, but it wasn't obvious how to expose such a flag via the engine API. While looking at the code it was noticed that issueCustomEndPut() is not called when running in standard-compliant refresh-from-client mode, because the code is only invoked at the end of the map phase, which is skipped in that mode. A bug?! Not fixed. commit 1da2ea00d5b09fd0c9c091cfd2a3ae1577206516 Author: Patrick Ohly <patrick.ohly(a)intel.com&gt; Date: Tue Feb 7 15:50:36 2012 +0100 datastore: explicitly tell the engine whether restarting a sync is supported The non-standard "multiple sync cycles in the same session" mode depends on data stores which support multiple read/write cycles and properly report changes at the start of additional cycles. The engine will automatically run multiple cycles if it deems that necessary to get client and server in sync. This commit introduces the necessary flag that must be set for a store before the engine can be sure that it is safe to run more than one cycle. Not currently checked by the engine, though! -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.

Hi Patrick, On Feb 13, 2012, at 14:35 , Patrick Ohly wrote: Thanks for these patches (the one with firstReadNextItem not being set is an embarassing one - on the other hand it proves safe coding in all of the plugins in use so far in that they apparently all cleanly reset the iterator at initialisation without being told so). Regarding the solution for the 409 merge problem, I think the fake-ID-delete approach is fine. Numeric localids are not used in any libsynthesis build, this is legacy from the monolithic PalmOS clients which had severe memory constraints which ruled out anything as fat and resource wasting as string IDs :-) Still, I think the use of negative IDs would work here, the only thing I'm not sure is if RND_MAX can be safely assumed to be 31 bits only - in case it would extend into 32 bit we'd get positive IDs clashing with real ones occasionally. Maybe just AND rand() result with 0x7FFFFFFF to be on the safe side... Best Regards, Lukas

On Tue, 2012-03-06 at 14:50 +0100, Patrick Ohly wrote: Done in the meego.gitorious.org master branch. I found that checking for collisions is hard (not all records are in memory), so I settled for making the chance of collisions smaller in the string case by including a running count. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.