Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
On Fri, 2009-07-24 at 20:20 +0200, Patrick Ohly wrote: Hello! Let's pick up this discussion again. When we agree on the API changes, then I'll try to follow up with an implementation for the file backend. On Thu, 2009-01-08 at 13:48 +0100, Patrick Ohly wrote: Hello Suman! I forgot to ask: do you agree in general with the plan to do atomic updates via e_book_commit_contact() and e_cal_modify_object() by defining the semantic as suggested? I've come to the conclusion that overloading the old calls is not worth the trouble. Therefore I now suggest to add new variants of the call because... I also need to extend the proposal: removing an item has similar race conditions (sync starts, user updates item, sync removes item). The current APIs for removing items make it harder to pass in the expected REV resp. LAST-MODIFIED. The only API call that could do it without changing its prototype is e_book_async_remove_contact(EContact *contact). e_book_remove_contact(), e_cal_remove_object(), e_cal_remove_object_with_mod() all just take ID strings. ... we need these new variants for the delete operations anyway. I have not looked at the calendar API yet. Let me know what you think about the attached patch for a new ebook API and I will continue with calendar next, before implementing anything. Then changes made are reasonable. Nice api documentation! This can be done in calendar also. For calendars we use sequence numbers for items shared with people and last-modified in general. In this current incarnation the patch tries a bit to provide simple implementations of the new calls, but this isn't meant to be complete. Would be better to get these api's committed after the branching is done for 2.27.x. We will be merging the eds-addressbook dbus port this week (for 2.27.x) and the calendar part for evolution 3.0. Will keep you informed on the same. - Chenthill. ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
Hello!
Let's pick up this discussion again. When we agree on the API changes,
then I'll try to follow up with an implementation for the file backend.
On Thu, 2009-01-08 at 13:48 +0100, Patrick Ohly wrote:
Hello Suman!
I forgot to ask: do you agree in general with the plan to do atomic
updates via e_book_commit_contact() and e_cal_modify_object() by
defining the semantic as suggested?
I've come to the conclusion that overloading the old calls is not worth
the trouble. Therefore I now suggest to add new variants of the call
because...
I also need to extend the proposal: removing an item has similar race
conditions (sync starts, user updates item, sync removes item). The
current APIs for removing items make it harder to pass in the expected
REV resp. LAST-MODIFIED. The only API call that could do it without
changing its prototype is e_book_async_remove_contact(EContact
*contact). e_book_remove_contact(), e_cal_remove_object(),
e_cal_remove_object_with_mod() all just take ID strings.
... we need these new variants for the delete operations anyway.
I have not looked at the calendar API yet. Let me know what you think
about the attached patch for a new ebook API and I will continue with
calendar next, before implementing anything.
In this current incarnation the patch tries a bit to provide simple
implementations of the new calls, but this isn't meant to be complete.
--
Bye, Patrick Ohly
--
patrick.o...@gmx.de
http://www.estamos.de/
commit 86120f2c129bac4e24ea3d96c8d458ab68301414
Author: Patrick Ohly patrick.o...@gmx.de
Date: Fri Jul 24 20:08:01 2009 +0200
atomic updates and deletes: check and return revision
The current API suffers from multiple race conditions: when
overwriting a contact, another client might have already made
an update that gets overwritten by older data. Removing a
contact is also affected = added an API extension that
allows backends a) to detect that clients want the more
careful data modifications and b) provides enough information
to do the checking based on the revision string in the backend
(EBOOK_STATIC_CAPABILITY_ATOMIC_MODIFICATIONS).
Software that does change tracking based on the revision string
(like SyncEvolution) needs to know which revision string was
assigned to an updated or added contact. Currently it must do
the operation, then ask for the whole contact. There is a small
window for a race condition here, but more important, this
requires another round-trip and transmit too much information.
With EBOOK_STATIC_CAPABILITY_RETURN_REV the client can be sure
to get the necessary information right away.
diff --git a/addressbook/libebook/e-book-types.h b/addressbook/libebook/e-book-types.h
index 00a814e..2dadf27 100644
--- a/addressbook/libebook/e-book-types.h
+++ b/addressbook/libebook/e-book-types.h
@@ -64,11 +64,23 @@ typedef enum {
E_BOOK_CHANGE_CARD_MODIFIED
} EBookChangeType;
+typedef enum {
+ E_BOOK_UNDEFINED_REVISION_HANDLING = 0,
+ E_BOOK_IGNORE_REVISION = 10,
+ E_BOOK_CHECK_REVISION = 11,
+ E_BOOK_SET_REVISION = 12
+} EBookRevisionHandlingFlags;
+
typedef struct {
EBookChangeType change_type;
EContact*contact;
} EBookChange;
+typedef struct {
+ const char *id;
+ const char *rev;
+} EBookContactInstance;
+
G_END_DECLS
#endif /* ! __E_BOOK_TYPES_H__ */
diff --git a/addressbook/libebook/e-book.c b/addressbook/libebook/e-book.c
index fefe4fa..07ab5d7 100644
--- a/addressbook/libebook/e-book.c
+++ b/addressbook/libebook/e-book.c
@@ -367,7 +367,10 @@ do_add_contact (gboolean sync,
* @contact: an #EContact
* @error: a #GError to set on failure
*
- * Adds @contact to @book.
+ * Adds @contact to @book. If the backend has the
+ * #EBOOK_STATIC_CAPABILITY_RETURN_REV, then the revision string
+ * assigned to the contact is guaranteed to be set in @contact when
+ * the function returns. The UID in @contact is set in all cases.
*
* Return value: %TRUE if successful, %FALSE otherwise.
**/
@@ -393,7 +396,10 @@ e_book_add_contact (EBook *book,
* @cb: function to call when the operation finishes
* @closure: data to pass to callback function
*
- * Adds @contact to @book without blocking.
+ * Adds @contact to @book without blocking. If the backend has the
+ * #EBOOK_STATIC_CAPABILITY_RETURN_REV, then the revision string
+ * assigned to the contact is guaranteed to be set in @contact when
+ * the operation completes. The UID in @contact is set in all cases.
*
* Return value: %TRUE if the operation was started, %FALSE otherwise.
**/
@@ -476,6 +482,7 @@ e_book_response_add_contact (EBook *book,
static gboolean
do_commit_contact (gbooleansync,
+ int flags,
EBook *book,
EContact *contact,
GError**error,
@@ -487,6 +494,12 @@ do_commit_contact (gbooleansync,
CORBA_Environment ev;
char *vcard_str;
+ /* callers must decide what to do with
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
On Thu, 2009-01-08 at 10:10 +0530, Suman Manjunath wrote: On Wed, Jan 7, 2009 at 17:15, Patrick Ohly patrick.o...@gmx.de wrote: The plan for change tracking is to get rid of the dependency on e_book_get_changes(). I already stopped using e_cal_get_changes() because it was too inflexible. Instead I'll rely on the REV resp. LAST-MODIFIED properties: the backend must update these each time an item is modified. This seems to be supported by most backends. Are there backends which are known to not support this? a) Looking at [1], I can't find what a REV property is. Did you mean to use [2] ? REV is part of vCard 3.0. It would be used in ebook. ecal would use LAST-MODIFIED instead. Does the GroupWise server update REV when contacts are modified? The problem with concurrent modifications is two-fold. Stale data in UI: * user opens an item in Evolution * synchronization starts in the background * updates the item in EDS * = When the user saves his changes, will he overwrite the more recent data in EDS? Will he be warned? With Evolution the user is not warned and his changes overwrite the ones in EDS (tested with contacts). Evolution should listen for change signals and warn the user as soon as he has stale data in the edit dialog. The user then can cancel and reopen the item to redo his changes. This is unlikely to happen often, so more elaborate solutions (merging changes, additional buttons to copy from EDS) should not be necessary. Should I file a bug for this? Anyone able and willing to work on it? Not so for calendars. I only tested with a contact. There the problem occurs as described above. Stale data in sync: * when the sync starts, it builds a list of new/updated/deleted items * user modifies data in EDS * this leads to conflicts, f.i. sync modifies item that was modified by user Both cases need to be handled by the program which wants to make changes to EDS data (Evolution, sync engine). To avoid race conditions, support by EDS would be needed which currently doesn't exist. As a workaround the following method would reduce the time window in which conflicts can occur: * get revstring before starting to make modifications (when opening item in UI; when starting sync) * before modifying the item, check the revstring again * if the same as before, do the modification * if different, handle conflict The GroupWise server updates the 'modified' property of the item when it actually gets modified on the server. For newly created items, it also adds the 'created' property at the same time. This behavior invalidates all the 'handle-at-backend' approaches to fix the apparent bug, the apparent bug = https://bugzilla.novell.com/show_bug.cgi?id=184554 ? Or do you mean the problem with overwriting more recent changes? I'm not sure I understood how the GroupWise server and backend work. What you are saying is that locally cached data might get out of sync with the server and thus checks in the Evolution backend cannot be implemented 100% accurately, right? That sounds like a real problem, but I still see room for improvement. For example, the backend could implement the workaround that I described in my email. A client can't do that because without changes in the backend it might read stale data, compare successfully, then push an update which overwrites modifications on the server. A secure solution would have to put the revision check into EDS itself to make the check and update atomic. The proposal is to add this check to e_book_commit_contact() and e_cal_modify_object(): * The caller is expected to include REV resp. LAST-MODIFIED as read from EDS earlier. * The EDS backend compares against the current value before updating the item. If there is a mismatch, the update is rejected with a suitable error code. * If the values are unset, the update is always executed. A backend may not have a LAST-MODIFIED property for a particular event in this use case: a) create a new appointment in the GW calendar (while online) b) open the same appointment (before the refresh timeout) That's because the local copy of the appointment is not identical to the one on the server, right? Perhaps the backend could force an immediate refresh of this particular appointment to get the server's LAST-MODIFIED? -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
Hello Suman! I forgot to ask: do you agree in general with the plan to do atomic updates via e_book_commit_contact() and e_cal_modify_object() by defining the semantic as suggested? I also need to extend the proposal: removing an item has similar race conditions (sync starts, user updates item, sync removes item). The current APIs for removing items make it harder to pass in the expected REV resp. LAST-MODIFIED. The only API call that could do it without changing its prototype is e_book_async_remove_contact(EContact *contact). e_book_remove_contact(), e_cal_remove_object(), e_cal_remove_object_with_mod() all just take ID strings. When e_cal_remove*() refers to multiple VEVENTs it might also be difficult to specify the expected LAST-MODIFIED of each VEVENT. I don't have a good solution for the remove case. New API calls? e_cal_modify_object() has the same problem with multiple VEVENTs, but this can be solved: for CALOBJ_MOD_THIS[ANDPRIOR|FUTURE], the LAST-MODIFIED property of the referenced VEVENT is to be compared. For CALOBJ_MOD_ALL the main event is checked. -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
On Wed, 2009-01-07 at 12:45 +0100, Patrick Ohly wrote: Hello! I'm currently thinking about synchronizing data with SyncEvolution in the background while the user is active with the Evolution UI. Some users already do that via cron jobs, but it is known to be problematic for several reasons, among them change tracking and concurrent editing of items. I already discussed this with Ross Burton and Rob Bradford. Now I would like to check that we haven't missed anything. The plan for change tracking is to get rid of the dependency on e_book_get_changes(). I already stopped using e_cal_get_changes() because it was too inflexible. Instead I'll rely on the REV resp. LAST-MODIFIED properties: the backend must update these each time an item is modified. This seems to be supported by most backends. Are there backends which are known to not support this? REV and LAST-MODIFIED are treated as opaque revision strings. SyncEvolution doesn't interpret the content, only compares it against the last synchronized value. For calendar, we just use last modified time. The problem with concurrent modifications is two-fold. Stale data in UI: * user opens an item in Evolution * synchronization starts in the background * updates the item in EDS * = When the user saves his changes, will he overwrite the more recent data in EDS? Will he be warned? With Evolution the user is not warned and his changes overwrite the ones in EDS (tested with contacts). Evolution should listen for change signals and warn the user as soon as he has stale data in the edit dialog. The user then can cancel and reopen the item to redo his changes. This is unlikely to happen often, so more elaborate solutions (merging changes, additional buttons to copy from EDS) should not be necessary. Should I file a bug for this? Anyone able and willing to work on it? Stale data in sync: * when the sync starts, it builds a list of new/updated/deleted items * user modifies data in EDS * this leads to conflicts, f.i. sync modifies item that was modified by user In both cases EDS signals modification of event to UI and all the clients listening on the ECalView.. The clients need to watch for this signal and update them accordingly. Both cases need to be handled by the program which wants to make changes to EDS data (Evolution, sync engine). To avoid race conditions, support by EDS would be needed which currently doesn't exist. As a workaround the following method would reduce the time window in which conflicts can occur: * get revstring before starting to make modifications (when opening item in UI; when starting sync) * before modifying the item, check the revstring again * if the same as before, do the modification * if different, handle conflict A secure solution would have to put the revision check into EDS itself to make the check and update atomic. The proposal is to add this check to e_book_commit_contact() and e_cal_modify_object(): * The caller is expected to include REV resp. LAST-MODIFIED as read from EDS earlier. * The EDS backend compares against the current value before updating the item. If there is a mismatch, the update is rejected with a suitable error code. * If the values are unset, the update is always executed. Problem 1: a client cannot tell whether the backend that it is using implements this check. If the backend doesn't (not yet updated, not possible), then the client should implement the workaround described above. Since the backends emit the notification, currently it assumed that the clients would handle it. I don think the backends check for the last modified time while updating... I think it should be handled both at the client side as well as EDS side to avoid race conditions. Problem 2: the backend cannot tell whether the client wants to have the check enabled or not. An older client might send values for REV/LAST-MODIFIED and might expect to always succeed. Such a client could be considered broken and refusing the update might be the right solution. Problem 3: the Exchange Connector does not update LAST-MODIFIED if the VEVENT contains it. I consider this a bug and work around it in SyncEvolution by filtering out the property before importing into EDS. Adding LAST-MODIFIED unconditionally would break change tracking. This should be a bug if exchange is not updating the last-modified time. Exact handling of REV and LAST-MODIFIED in the two calls is basically undefined right now as far as I can tell. I suggest to clarify that: * backends should check them as described above * backends must update them independently of what the client sends To allow clients to query whether the backend supports the check I suggest to add a new
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
On Thu, Jan 8, 2009 at 15:19, Patrick Ohly patrick.o...@gmx.de wrote: The GroupWise server updates the 'modified' property of the item when it actually gets modified on the server. For newly created items, it also adds the 'created' property at the same time. This behavior invalidates all the 'handle-at-backend' approaches to fix the apparent bug, the apparent bug = https://bugzilla.novell.com/show_bug.cgi?id=184554 ? Yep.. it is this ^^ bug. And I agree with Chen that it needs a fix at the backend. -Suman ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
[Evolution-hackers] concurrent modifications of items in GUI and EDS database
Hello! I'm currently thinking about synchronizing data with SyncEvolution in the background while the user is active with the Evolution UI. Some users already do that via cron jobs, but it is known to be problematic for several reasons, among them change tracking and concurrent editing of items. I already discussed this with Ross Burton and Rob Bradford. Now I would like to check that we haven't missed anything. The plan for change tracking is to get rid of the dependency on e_book_get_changes(). I already stopped using e_cal_get_changes() because it was too inflexible. Instead I'll rely on the REV resp. LAST-MODIFIED properties: the backend must update these each time an item is modified. This seems to be supported by most backends. Are there backends which are known to not support this? REV and LAST-MODIFIED are treated as opaque revision strings. SyncEvolution doesn't interpret the content, only compares it against the last synchronized value. The problem with concurrent modifications is two-fold. Stale data in UI: * user opens an item in Evolution * synchronization starts in the background * updates the item in EDS * = When the user saves his changes, will he overwrite the more recent data in EDS? Will he be warned? With Evolution the user is not warned and his changes overwrite the ones in EDS (tested with contacts). Evolution should listen for change signals and warn the user as soon as he has stale data in the edit dialog. The user then can cancel and reopen the item to redo his changes. This is unlikely to happen often, so more elaborate solutions (merging changes, additional buttons to copy from EDS) should not be necessary. Should I file a bug for this? Anyone able and willing to work on it? Stale data in sync: * when the sync starts, it builds a list of new/updated/deleted items * user modifies data in EDS * this leads to conflicts, f.i. sync modifies item that was modified by user Both cases need to be handled by the program which wants to make changes to EDS data (Evolution, sync engine). To avoid race conditions, support by EDS would be needed which currently doesn't exist. As a workaround the following method would reduce the time window in which conflicts can occur: * get revstring before starting to make modifications (when opening item in UI; when starting sync) * before modifying the item, check the revstring again * if the same as before, do the modification * if different, handle conflict A secure solution would have to put the revision check into EDS itself to make the check and update atomic. The proposal is to add this check to e_book_commit_contact() and e_cal_modify_object(): * The caller is expected to include REV resp. LAST-MODIFIED as read from EDS earlier. * The EDS backend compares against the current value before updating the item. If there is a mismatch, the update is rejected with a suitable error code. * If the values are unset, the update is always executed. Problem 1: a client cannot tell whether the backend that it is using implements this check. If the backend doesn't (not yet updated, not possible), then the client should implement the workaround described above. Problem 2: the backend cannot tell whether the client wants to have the check enabled or not. An older client might send values for REV/LAST-MODIFIED and might expect to always succeed. Such a client could be considered broken and refusing the update might be the right solution. Problem 3: the Exchange Connector does not update LAST-MODIFIED if the VEVENT contains it. I consider this a bug and work around it in SyncEvolution by filtering out the property before importing into EDS. Adding LAST-MODIFIED unconditionally would break change tracking. Exact handling of REV and LAST-MODIFIED in the two calls is basically undefined right now as far as I can tell. I suggest to clarify that: * backends should check them as described above * backends must update them independently of what the client sends To allow clients to query whether the backend supports the check I suggest to add a new capability for e_cal_get_static_capability() and e_book_check_static_capabilities(). Any comments? -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
Il giorno mer, 07/01/2009 alle 12.45 +0100, Patrick Ohly ha scritto: Hello! I'm currently thinking about synchronizing data with SyncEvolution in the background while the user is active with the Evolution UI. Some users already do that via cron jobs, but it is known to be problematic for several reasons, among them change tracking and concurrent editing of items. I started studing a plugin to do the sync on user request. I'm going to present my idea in another email just to have clearer threads. Bye sc -- Stefano Canepa aka sc: s...@linux.it - http://www.stefanocanepa.it Three great virtues of a programmer: laziness, impatience and hubris. Le tre grandi virtù di un programmatore: pigrizia, impazienza e arroganza. (Larry Wall) signature.asc Description: Questa è una parte del messaggio firmata digitalmente ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] concurrent modifications of items in GUI and EDS database
On Wed, Jan 7, 2009 at 17:15, Patrick Ohly patrick.o...@gmx.de wrote: The plan for change tracking is to get rid of the dependency on e_book_get_changes(). I already stopped using e_cal_get_changes() because it was too inflexible. Instead I'll rely on the REV resp. LAST-MODIFIED properties: the backend must update these each time an item is modified. This seems to be supported by most backends. Are there backends which are known to not support this? a) Looking at [1], I can't find what a REV property is. Did you mean to use [2] ? The problem with concurrent modifications is two-fold. Stale data in UI: * user opens an item in Evolution * synchronization starts in the background * updates the item in EDS * = When the user saves his changes, will he overwrite the more recent data in EDS? Will he be warned? With Evolution the user is not warned and his changes overwrite the ones in EDS (tested with contacts). Evolution should listen for change signals and warn the user as soon as he has stale data in the edit dialog. The user then can cancel and reopen the item to redo his changes. This is unlikely to happen often, so more elaborate solutions (merging changes, additional buttons to copy from EDS) should not be necessary. Should I file a bug for this? Anyone able and willing to work on it? Not so for calendars. When an event is open in the Evolution UI and the backend modifies it during a refresh/update from server, the user _is_ warned of an update to the item. If not, the backend is probably not doing a e_cal_backend_notify_object_modified() which it ought to. FWIW, see the open bug at [3]. Stale data in sync: * when the sync starts, it builds a list of new/updated/deleted items * user modifies data in EDS * this leads to conflicts, f.i. sync modifies item that was modified by user Both cases need to be handled by the program which wants to make changes to EDS data (Evolution, sync engine). To avoid race conditions, support by EDS would be needed which currently doesn't exist. As a workaround the following method would reduce the time window in which conflicts can occur: * get revstring before starting to make modifications (when opening item in UI; when starting sync) * before modifying the item, check the revstring again * if the same as before, do the modification * if different, handle conflict The GroupWise server updates the 'modified' property of the item when it actually gets modified on the server. For newly created items, it also adds the 'created' property at the same time. This behavior invalidates all the 'handle-at-backend' approaches to fix the apparent bug, like: * Check if the item is newly-created by looking for a 'modified' property in the cached-object. This approach does not handle modifying an older (and already cached from server) item. * Pushing the 'modified' property from the client to the server upon creation/modification. This does not help, since the server would modify the property anyway. * A series of 'if-else' trying to handle various scenarios. It was observed that the conditions would fail for at-least one use-case, mainly since we would not have a persistent 'last-modified' time unless it is obtained from the server. These drawbacks probably apply to the Exchange backend too. A secure solution would have to put the revision check into EDS itself to make the check and update atomic. The proposal is to add this check to e_book_commit_contact() and e_cal_modify_object(): * The caller is expected to include REV resp. LAST-MODIFIED as read from EDS earlier. * The EDS backend compares against the current value before updating the item. If there is a mismatch, the update is rejected with a suitable error code. * If the values are unset, the update is always executed. A backend may not have a LAST-MODIFIED property for a particular event in this use case: a) create a new appointment in the GW calendar (while online) b) open the same appointment (before the refresh timeout) Pushing an update in this case is not correct as the event can easily be modified by another client during that refresh interval. We should ensure that the cached object in EDS will always have the same last-modified property as that on the server before opening the event in the UI [This probably implies an (expensive?) 'live' re-cache of the object being opened]. We already listen for changes to the event and so the user automatically knows if anything changed since opening the event editor. We don't continuously listen for updates from the server and hence the problem I mentioned above would still exist, but that is something I can live with :) - Suman [1] http://tools.ietf.org/html/rfc2445 [2] http://tools.ietf.org/html/rfc2445#section-4.8.7.4 [3]
