Hello Patrick,
On Dec 9, 2010, at 17:47 , Patrick Ohly wrote:
> I might have found it.
>
> Scenario:
> - SERVER is the ID on the server, CLIENT on the client
> - server has a mapping from SERVER to #35 (ident 2) and to CLIENT (ident 1)
Where is this from? I assume it is from a previous session (that has not
completed, otherwise it should not be there). That would be ok.
Strictly following the SyncML specs, sending the SERVER ID (or a temp ID
representing it) to the client is only needed for <Add> (to allow mapping
back). However, because the engine has the <alwayssendlocalid> config option,
it always calculates the temp ID, even if the option is off. This is something
that could be optimized (but it would hide the bug we're after, so that's an
exercise for later :-)
> - item is deleted on server
> - two-way sync
>
> What I now see in the log is this:
> fTempGUIDMap: restore mapping from #35 to SERVER
Whatever origin that is and whether it makes sense here or not, I agree that
this can happen and should not cause problems.
> Item LocalID='SERVER', RemoteID='CLIENT', operation=delete
> SyncOp=delete: LocalID=SERVER RemoteID=CLIENT
> fTempGUIDMap: translated realLocalID='SERVER' to tempLocalID='#63'
By itself, this is correct. The temp mapping #35->SERVER that was loaded at the
beginning of the sync is invalid by now (transferring data for this session has
begun). So basically it is ok that the engine generates a new temp mapping.
What surprised me is that the old #35->SERVER mapping still exists in
fTempGUIDMap at this point. As I said earlier, fTempGUIDMap is cleared once per
session before items are exchanged. But not at the very beginning of the
session, because a client might send pending maps left over from the previous
session.
Now it turns out that there's a bad bug here, which probably explains all this
(and maybe other) weird behaviour. Fact is that fTempGUIDMap gets cleared way
too early, that is BEFORE apiLoadAdminData is called. Which means that old and
new tempGUIDs get mixed here. And that fTempGUIDMap can become a more or less
ever-growing list (growth however limited a lot by the very collisions we are
talking about).
The clearing of fTempGUIDMap must happen at the point where the server proceses
the <Sync> command, that is when exchange of this session's items starts. This
is before the datastore changes state from dssta_syncmodestable to
dssta_dataaccessstarted.
That's what I did now (again not tested yet because I have to run out of the
office now). But I wanted to share the finding already, so here's a patch:
>From a5e1bd5241380d6f2e19e1affdc1002d71a638f4 Mon Sep 17 00:00:00 2001
From: Lukas Zeller <l...@plan44.ch>
Date: Thu, 16 Dec 2010 18:32:02 +0100
Subject: [PATCH] engine: server case: fixed bad bug that could mess up
tempGUIDs. These must be cleared when first <Sync> is received.
Until now, the tempGUIDs were cleared only once when <Alert> was received.
This is ok to create a clean starting point for loading previous session's
tempGUIDs, which might be needed to resolve "early maps" also related to
the previous session.
However, as soon as the current session starts <Sync>ing, that is,
exchanging new items, these temporary IDs become invalid and
fTempGUIDMap must be cleared once again.
The missing cleanup has led to collisions in the tempGUID name space,
because the tempGUIDs are created based on the fact that the list
is empty at the beginning of <Sync>. As it could have entries
from (possibly much!) earlier sessions, all sort of weird things
could and did happen.
Thanks Patrick for the extensive investigation of this!
---
sysync/localengineds.cpp | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/sysync/localengineds.cpp b/sysync/localengineds.cpp
index ba8307f..b084cb4 100755
--- a/sysync/localengineds.cpp
+++ b/sysync/localengineds.cpp
@@ -2424,8 +2424,11 @@ TAlertCommand *TLocalEngineDS::engProcessSyncAlert(
// if we process a sync alert now, we haven't started sync or map
generation
#ifdef SYSYNC_SERVER
if (IS_SERVER) {
- // server case: forget Temp GUID mapping
- fTempGUIDMap.clear(); // forget all previous temp GUID mappings
+ // make sure we are not carrying forward any left-overs. Last sessions's
tempGUID mappings that are
+ // needed for "early map" resolution might be loaded by the call to
engInitSyncAnchors below.
+ // IMPORTANT NOTE: the tempGUIDs that might get loaded will become
invalid as soon as <Sync>
+ // starts - so fTempGUIDMap needs to be cleared again as soon as the
first <Sync> command arrives from the client.
+ fTempGUIDMap.clear();
}
#endif
// save remote's next anchor for saving at end of session
@@ -3902,7 +3905,9 @@ bool TLocalEngineDS::engProcessSyncCmd(
startingNow = true; // initiating start now
#ifdef SYSYNC_SERVER
if (IS_SERVER) {
- // - let local datastore (derived DB-specific class) prepare for sync
+ // at this point, all temporary GUIDs become invalid (no "early map"
possible any more which might refer to last session's tempGUIDs)
+ fTempGUIDMap.clear(); // forget all previous session's temp GUID mappings
+ // let local datastore (derived DB-specific class) prepare for sync
localstatus sta = changeState(dssta_dataaccessstarted);
if (sta!=LOCERR_OK) {
// abort session (old comment: %%% aborting datastore only does not
work, will loop, why? %%%)
--
1.7.2.3+GitX
Best Regards,
Lukas
_______________________________________________
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
