1:55 a.m.
Hello!
A SyncEvolution user found that SyncEvolution fails while synchronizing
against a mobile phone when an item was deleted on both the phone and
Evolution side. SyncEvolution is asked to read the already deleted item,
which then fails (currently) with an unspecified database error.
This is due to the following code:
#20 0x00000000009b89de in sysync::TSuperDataStore::engProcessRemoteItem (this=0x1166900,
syncitemP=0xe854e0,
aStatusCommand=...) at
/home/pohly/syncevolution/libsynthesis/src/sysync/superdatastore.cpp:637
637
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
(gdb) list
632 PDEBUGPRINTFX(DBG_DATA+DBG_DETAILS,(
633 "Trying to read item by remoteID='%s' from subdatastore
'%s' to see if it is there",
634 itemcopyP->getRemoteID(),
635 linkP->fDatastoreLinkP->getName()
636 ));
637
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
638 // must be ok AND not 404 (item not found)
639 if (regular && aStatusCommand.getStatusCode()!=404) {
640 PDEBUGPRINTFX(DBG_DATA,(
641 "Item found in subdatastore '%s', deleting it there",
Why is it necessary to read before trying to delete? If the item exists,
then reading it is a fairly expensive test.
So far, my backends were written with the expectation that they have to
cope with delete requests for items which are already deleted. This
follows from the inherent race condition between syncing and some other
process which might delete items while a sync runs.
Are all backends expected to return 404 when an item doesn't exist?
--
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.
3:07 a.m.
On Mon, 2011-10-24 at 16:55 +0200, Patrick Ohly wrote:
Are all backends expected to return 404 when an item doesn't
exist?
I have a patch ready for Evolution contacts and calendar ready which
does this. However, it is not very nice: in order to avoid confusing
ERROR messages to the user, the ReadItem callback now has to suppress
404 logging in all cases, regardless whether such an error is acceptable
or a real error.
--
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.
6:24 p.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
Hello Patrick,
On Oct 24, 2011, at 16:55 , Patrick Ohly wrote:
Why is it necessary to read before trying to delete? If the item
exists,
then reading it is a fairly expensive test.
Unfortunately, with some some backends, this is the only test that reliably works. For
example, some SQL databases (or their ODBC driver) can't return a correct "number
of affected rows" for DELETE statements. So reading before deleting was the only way
to detect if any of the subdatastores really had that item and correctly return 404 or
200.
So far, my backends were written with the expectation that they have
to
cope with delete requests for items which are already deleted. This
follows from the inherent race condition between syncing and some other
process which might delete items while a sync runs.
Are all backends expected to return 404 when an item doesn't exist?
Not all backends (see above, built-in ODBC can't), but yes, for plugin backends,
returning proper error codes is specified, also for delete. Still, if a plugin would not
conform to that in its implementation of delete, that would probably have gone unnoticed
so far.
Of course, the test is a bit expensive - if that's a concern, one could differentiate
between plugin datastores and others (all the builtin ones, including SQL/ODBC), and use
the expensive read test only for the latter. Like a virtual dsDeleteDetectsItemPresence()
which returns false by default, but can be overridden in plugin datastores to return
true.
Best Regards,
Lukas Zeller, plan44.ch
luz(a)plan44.ch - www.plan44.ch
6:36 p.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
On Tue, 2011-10-25 at 09:24 +0200, Lukas Zeller wrote:
Hello Patrick,
On Oct 24, 2011, at 16:55 , Patrick Ohly wrote:
> Why is it necessary to read before trying to delete? If the item exists,
> then reading it is a fairly expensive test.
Unfortunately, with some some backends, this is the only test that
reliably works. For example, some SQL databases (or their ODBC driver)
can't return a correct "number of affected rows" for DELETE
statements. So reading before deleting was the only way to detect if
any of the subdatastores really had that item and correctly return 404
or 200.
> So far, my backends were written with the expectation that they have to
> cope with delete requests for items which are already deleted. This
> follows from the inherent race condition between syncing and some other
> process which might delete items while a sync runs.
>
> Are all backends expected to return 404 when an item doesn't exist?
Not all backends (see above, built-in ODBC can't), but yes, for plugin
backends, returning proper error codes is specified, also for delete.
Are these expectations documented somewhere? I'm still a bit fuzzy about
which error codes are expected. For example, should a delete of a
non-existent item succeed (200) or fail (404)?
Still, if a plugin would not conform to that in its implementation
of
delete, that would probably have gone unnoticed so far.
Right. SyncEvolution has hardly ever (never?) returned 404 and that has
not been an issue until now ;-)
Of course, the test is a bit expensive - if that's a concern,
one
could differentiate between plugin datastores and others (all the
builtin ones, including SQL/ODBC), and use the expensive read test
only for the latter. Like a virtual dsDeleteDetectsItemPresence()
which returns false by default, but can be overridden in plugin
datastores to return true.
That sounds like the right compromise. I'm much more comfortable
returning 404 in a delete and not reporting that to the user as an
error, compared to not reporting a 404 in a ReadItem call (as I have to
do now).
--
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.
7:16 p.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
Hello Patrick,
On Oct 25, 2011, at 9:36 , Patrick Ohly wrote:
Are these expectations documented somewhere?
Probably not as clearly as it should be. Beat's doc/SDK_manual.pdf is all we have.
I'm still a bit fuzzy about
which error codes are expected. For example, should a delete of a
non-existent item succeed (200) or fail (404)?
It should return 404. The basic rule is: at the API level, the errors should be as direct
and "honest" as possible.
If the engine tries to delete something that does not exist, the plugin should return an
error. It's the task of the engine to disguise that error towards the SyncML peer (and
in fact, there is an option for that: <deletinggoneok>, on by default, which means
that failed deletes don't show up as 404 towards the peer, but go through as 200/OK).
> Still, if a plugin would not conform to that in its
implementation of
> delete, that would probably have gone unnoticed so far.
Right. SyncEvolution has hardly ever (never?) returned 404 and that has
not been an issue until now ;-)
Because <deletinggoneok> is on by default, it made no difference. However (and
that's the reason for that expensive test in the first place and the config option),
the SCTS (SyncML Conformance Test Suite) did a test which failed when deleting a
non-existing item returned 200. As in the SyncFest's back in early 2000s passing SCTS
was a prerequiste to be allowed to go to the IOT, the demands of SCTS had to be met even
if the opposite made more sense for real world operation...
> Of course, the test is a bit expensive - if that's a concern,
one
> could differentiate between plugin datastores and others (all the
> builtin ones, including SQL/ODBC), and use the expensive read test
> only for the latter. Like a virtual dsDeleteDetectsItemPresence()
> which returns false by default, but can be overridden in plugin
> datastores to return true.
That sounds like the right compromise. I'm much more comfortable
returning 404 in a delete and not reporting that to the user as an
error, compared to not reporting a 404 in a ReadItem call (as I have to
do now).
Now I don't understand. Why do you have to *not* report a 404 in ReadItem now??
Best Regards,
Lukas Zeller, plan44.ch
luz(a)plan44.ch - www.plan44.ch
7:46 p.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
On Tue, 2011-10-25 at 10:16 +0200, Lukas Zeller wrote:
> That sounds like the right compromise. I'm much more
comfortable
> returning 404 in a delete and not reporting that to the user as an
> error, compared to not reporting a 404 in a ReadItem call (as I have to
> do now).
Now I don't understand. Why do you have to *not* report a 404 in
ReadItem now??
Reporting to the engine is fine, reporting to the user is the
problematic part. Let me explain.
The SyncEvolution output, the one that is visible to end-users, contains
INFO and ERROR messages about operations happening in the backend. Each
attempt by the Synthesis engine to read a non-existent item results in a
user-visible error. Or rather, two of them in the case of a super data
store combining "calendar" and "todo":
[ERROR] error code from SyncEvolution fatal error (local, status 10500): calendar:
retrieving item: 20111023T082825Z-2322-1001-1969-0@lulu-rid: Objektpfad des Kalenders kann
nicht abgerufen werden: Objekt nicht gefunden
[ERROR] error code from SyncEvolution fatal error (local, status 10500): todo: retrieving
item: 20111023T082825Z-2322-1001-1969-0@lulu-rid: Objektpfad des Kalenders kann nicht
abgerufen werden: Objekt nicht gefunden
At the point where that logging happens it is unknown whether the error
is really a problem or can be ignored. Only the Synthesis engine itself
knows. When I started to use the Synthesis engine, I tried to make the
logging happening inside the engine visible to users, but it simply
wasn't meant to be used for that and so I gave up on that approach.
Admittedly the ERROR message above is not very informative either.
--
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.
12:46 a.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
Hello Patrick,
On Oct 25, 2011, at 10:46 , Patrick Ohly wrote:
> Now I don't understand. Why do you have to *not* report a 404
in
> ReadItem now??
Reporting to the engine is fine, reporting to the user is the
problematic part. Let me explain.
The SyncEvolution output, the one that is visible to end-users, contains
INFO and ERROR messages about operations happening in the backend. Each
attempt by the Synthesis engine to read a non-existent item results in a
user-visible error. Or rather, two of them in the case of a super data
store combining "calendar" and "todo":
[ERROR] error code from SyncEvolution fatal error (local, status 10500): calendar:
retrieving item: 20111023T082825Z-2322-1001-1969-0@lulu-rid: Objektpfad des Kalenders kann
nicht abgerufen werden: Objekt nicht gefunden
[ERROR] error code from SyncEvolution fatal error (local, status 10500): todo: retrieving
item: 20111023T082825Z-2322-1001-1969-0@lulu-rid: Objektpfad des Kalenders kann nicht
abgerufen werden: Objekt nicht gefunden
At the point where that logging happens it is unknown whether the error
is really a problem or can be ignored. Only the Synthesis engine itself
knows. When I started to use the Synthesis engine, I tried to make the
logging happening inside the engine visible to users, but it simply
wasn't meant to be used for that and so I gave up on that approach.
Admittedly the ERROR message above is not very informative either.
Ok, that is a bit of context needed to understand the initial statement :-)
But if the superdatastore was changed from probing with ReadItem to directly
probing/deleting with DeleteItem, the error reporting would look similar, as it would show
failed deletes for those subdatastores not containing the object in question.
I mean, for deleting a todo from the calendar superdatastore item you'd see something
like "calendar: failed deleting item xy" and "todo: deleted item xy
ok" (don't know if success is shown as well). Would this be any better / more
understandable?
Best Regards,
Lukas Zeller, plan44.ch
luz(a)plan44.ch - www.plan44.ch
1:32 a.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
On Tue, 2011-10-25 at 15:46 +0200, Lukas Zeller wrote:
But if the superdatastore was changed from probing with ReadItem to
directly probing/deleting with DeleteItem, the error reporting would
look similar, as it would show failed deletes for those subdatastores
not containing the object in question.
No, because I never consider attempts to delete a non-existent item as
an error and thus would always suppress the error message. After all,
the desired outcome was achieved.
This is different for reading where not getting the item data may or may
not be relevant.
--
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.
1:56 a.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
On Oct 25, 2011, at 16:32 , Patrick Ohly wrote:
On Tue, 2011-10-25 at 15:46 +0200, Lukas Zeller wrote:
> But if the superdatastore was changed from probing with ReadItem to
> directly probing/deleting with DeleteItem, the error reporting would
> look similar, as it would show failed deletes for those subdatastores
> not containing the object in question.
No, because I never consider attempts to delete a non-existent item as
an error and thus would always suppress the error message.
But still return 404 to the engine, I assume.
After all, the desired outcome was achieved.
This is different for reading where not getting the item data may or may
not be relevant.
Yes, seen from a standpoint where the result of the operation counts, and not the process,
a failing read is relevant while a failing delete is not - the desired result for the
latter (item gone) is ok, even if the item has disappeared sometime before.
Makes all perfect sense to me now :-)
Lukas Zeller, plan44.ch
luz(a)plan44.ch - www.plan44.ch
2:42 a.m.
New subject: [SyncEvolution] [os-libsynthesis] super data store: trying to read to check for existance
On Tue, 2011-10-25 at 09:24 +0200, Lukas Zeller wrote:
On Oct 24, 2011, at 16:55 , Patrick Ohly wrote:
> Why is it necessary to read before trying to delete? If the item exists,
> then reading it is a fairly expensive test.
Unfortunately, with some some backends, this is the only test that reliably works. For
example, some SQL databases (or their ODBC driver) can't return a correct "number
of affected rows" for DELETE statements. So reading before deleting was the only way
to detect if any of the subdatastores really had that item and correctly return 404 or
200.
> So far, my backends were written with the expectation that they have to
> cope with delete requests for items which are already deleted. This
> follows from the inherent race condition between syncing and some other
> process which might delete items while a sync runs.
>
> Are all backends expected to return 404 when an item doesn't exist?
Not all backends (see above, built-in ODBC can't), but yes, for plugin
backends, returning proper error codes is specified, also for delete.
Still, if a plugin would not conform to that in its implementation of
delete, that would probably have gone unnoticed so far.
I checked the API docs and did not see it mentioned explicitly.
In SyncEvolution I added some remarks about 404 (will probably not be
noticed) and also added automated Client::Source tests for it (which is
harder to ignore if someone runs the tests ;-)
I also changed the error logging so that 404 is reported to users in
ReadItem and not logged in DeleteItem (while still returned to the
engine).
Of course, the test is a bit expensive - if that's a concern,
one
could differentiate between plugin datastores and others (all the
builtin ones, including SQL/ODBC), and use the expensive read test
only for the latter. Like a virtual dsDeleteDetectsItemPresence()
which returns false by default, but can be overridden in plugin
datastores to return true.
The patch below implements that idea (from the for-master/bmc23744
branch). SyncEvolution relies on that patch to avoid the (harmless)
ERROR messages for "item not found" in ReadItem(), although everything
should work okay also without the patch.
Does that look right?
From fb5cc0dc19fb60e40a4ca2dcfe44a52a75ec7354 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri, 28 Oct 2011 15:42:57 +0200
Subject: [PATCH] server engine: more efficient deletion in superdatastore
Attempting to read an item just to check whether it exists is expensive.
It also may trigger error messages when the item does not exist (done by
SyncEvolution).
Therefore, if a data store is able to properly report whether it found
the item which is to be deleted, a different logic is used for
deletion in the superdatastore:
- attempt to delete until it succeeds in a subdatastore
- recreate the sync item from the copy after a failed delete attempt
(which deletes the sync item), if there is another loop iteration
Deleting the copy of the sync item was moved to the function exit code
to avoid code duplication.
By default all data stores are assumed to not support 404 in its
delete operation. The only exception is the plugin API. The (somewhat
undocumented) assumption is that all methods properly report 404 when
the requested item does not exist. The "attempt to read" code already
relied on that. Now the code relies on that for the "attempt to
delete".
Probably it even works when the plugin returns some other error
code (the "regular" value will be false in that case) or no error code
at all: the translation code between remote ID and local ID will
already detect that the item is not in the mapping table if it is not
in the subdatastore. So the actual plugin will not get called at all
in the case where its expected behavior would matter.
---
src/DB_interfaces/api_db/pluginapids.h | 4 ++
src/sysync/superdatastore.cpp | 80 ++++++++++++++++++++++----------
src/sysync/syncdatastore.h | 2 +
3 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/src/DB_interfaces/api_db/pluginapids.h
b/src/DB_interfaces/api_db/pluginapids.h
index 4fbfc79..f215094 100755
--- a/src/DB_interfaces/api_db/pluginapids.h
+++ b/src/DB_interfaces/api_db/pluginapids.h
@@ -172,6 +172,10 @@ public:
virtual void dsResetDataStore(void) { InternalResetDataStore();
inherited::dsResetDataStore(); };
virtual ~TPluginApiDS();
+ // override TSyncDataStore: the plugin must be able to return 404
+ // when an item is not found during delete
+ virtual bool dsDeleteDetectsItemPresence() const { return true; }
+
#ifndef BINFILE_ALWAYS_ACTIVE
/// @name apiXXXX methods defining the interface from TCustomImplDS to TXXXApi actual
API implementations
/// @{
diff --git a/src/sysync/superdatastore.cpp b/src/sysync/superdatastore.cpp
index e70d7bb..5c0a2b5 100755
--- a/src/sysync/superdatastore.cpp
+++ b/src/sysync/superdatastore.cpp
@@ -530,7 +530,7 @@ bool TSuperDataStore::engProcessRemoteItem(
bool regular=true;
string datatext;
#ifdef SYSYNC_SERVER
- TSyncItem *itemcopyP;
+ TSyncItem *itemcopyP = NULL;
#endif
// show
@@ -617,42 +617,71 @@ bool TSuperDataStore::engProcessRemoteItem(
case sop_delete:
case sop_copy:
// item has no local ID AND no data, only a remoteID:
- // we must try to read item from all subdatastores by remoteID until
- // one is found
+ // we must try to read item or attempt to delete in all subdatastores by
remoteID
+ // until found in one of them
+
// get an empty item of correct type to call logicRetrieveItemByID
itemcopyP = getLocalReceiveType()->newSyncItem(getRemoteSendType(),this);
// - only remote ID is relevant, leave everything else empty
itemcopyP->setRemoteID(syncitemP->getRemoteID());
- // try to read item from all subdatastores
+ // try to find item in any of the subdatastores
for (pos=fSubDSLinks.begin();pos!=fSubDSLinks.end();pos++) {
linkP = &(*pos);
// always start with 200
aStatusCommand.setStatusCode(200);
- // now try to read
- PDEBUGPRINTFX(DBG_DATA+DBG_DETAILS,(
- "Trying to read item by remoteID='%s' from subdatastore
'%s' to see if it is there",
- itemcopyP->getRemoteID(),
- linkP->fDatastoreLinkP->getName()
- ));
-
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
- // must be ok AND not 404 (item not found)
- if (regular && aStatusCommand.getStatusCode()!=404) {
- PDEBUGPRINTFX(DBG_DATA,(
- "Item found in subdatastore '%s', deleting it there",
+
+ // item deleted by a failed engProcessRemoteItem()?
+ if (!syncitemP) {
+ // recreate it for next attempt
+ syncitemP = getLocalReceiveType()->newSyncItem(getRemoteSendType(),this);
+ syncitemP->setRemoteID(itemcopyP->getRemoteID());
+ syncitemP->setSyncOp(sop);
+ }
+
+ if (linkP->fDatastoreLinkP->dsDeleteDetectsItemPresence()) {
+ // attempt to delete, consuming original item on success
+
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
+ // must be ok AND not 404 (item not found)
+ if (regular && aStatusCommand.getStatusCode()!=404) {
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Item found in subdatastore '%s', deleted it there",
+ linkP->fDatastoreLinkP->getName()
+ ));
+
+ // done
+ goto done;
+ } else {
+ // syncitemP was deleted by engProcessRemoteItem() (for
+ // example, in TStdLogicDS::logicProcessRemoteItem()),
+ // so we must remember to recreate it from the copy for
+ // another attempt if there is one
+ syncitemP = NULL;
+ }
+ } else {
+ // try to read first to determine whether the subdatastore contains the
item;
+ // necessary because the subdatastore is not able to report a 404 error in
+ // the delete operation when the item does not exist
+ PDEBUGPRINTFX(DBG_DATA+DBG_DETAILS,(
+ "Trying to read item by remoteID='%s' from subdatastore
'%s' to see if it is there",
+ itemcopyP->getRemoteID(),
linkP->fDatastoreLinkP->getName()
));
- // now we can delete or copy, consuming original item
-
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
- // delete duplicated item as well
- delete itemcopyP;
- // done
- regular=true;
- goto done;
+
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
+ // must be ok AND not 404 (item not found)
+ if (regular && aStatusCommand.getStatusCode()!=404) {
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Item found in subdatastore '%s', deleting it there",
+ linkP->fDatastoreLinkP->getName()
+ ));
+ // now we can delete or copy, consuming original item
+
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
+ // done
+ regular=true;
+ goto done;
+ }
}
}
// none of the datastores could process this item --> error
- // - delete duplicated item
- delete itemcopyP;
// - make sure delete reports 200 for incomplete-rollback-datastores
if (aStatusCommand.getStatusCode()==404 && sop!=sop_copy) {
// not finding an item for delete might be ok for remote...
@@ -722,6 +751,9 @@ nods:
regular=false;
goto done;
done:
+ #ifdef SYSYNC_SERVER
+ delete itemcopyP;
+ #endif
PDEBUGENDBLOCK("SuperProcessItem");
return regular;
} // TSuperDataStore::engProcessRemoteItem
diff --git a/src/sysync/syncdatastore.h b/src/sysync/syncdatastore.h
index 78e4559..0672341 100755
--- a/src/sysync/syncdatastore.h
+++ b/src/sysync/syncdatastore.h
@@ -56,6 +56,8 @@ public:
virtual uInt16 isDatastore(const char *aDatastoreURI);
// - returns if specified type is used by this datastore
bool doesUseType(TSyncItemType *aSyncItemType, TTypeVariantDescriptor
*aVariantDescP=NULL);
+ // - true if data store is able to return 404 when asked to delete non-existent item
+ virtual bool dsDeleteDetectsItemPresence() const { return false; }
// - get common types to send to or receive from another datastore
TSyncItemType *getTypesForTxTo(TSyncDataStore *aDatastoreP, TSyncItemType
**aCorrespondingTypePP=NULL);
TSyncItemType *getTypesForRxFrom(TSyncDataStore *aDatastoreP, TSyncItemType
**aCorrespondingTypePP=NULL);
--
1.7.2.5
3978
days inactive
3982
days old
syncevolution@synevolution.org
9 comments
2 participants
participants (2)
-
Lukas Zeller -
Patrick Ohly