[Syncevolution-issues] [Bug 1354] New: EDS: protect against concurrent editing + revision handling

http://bugs.meego.com/show_bug.cgi?id=1354 Summary: EDS: protect against concurrent editing + revision handling Classification: MeeGo Projects Product: SyncEvolution Version: unspecified Platform: Netbook OS/Version: IA Status: ASSIGNED Severity: normal Priority: High Component: SyncEvolution AssignedTo: ross(a)linux.intel.com ReportedBy: jingke.zhang(a)intel.com QAContact: jingke.zhang(a)intel.com CC: syncevolution-bugs(a)meego.bugs, syncevolution-default-bugs(a)meego.bugs Estimated Hours: 0.0 This is from http://bugzilla.moblin.org/show_bug.cgi?id=3479 Description From pohly 2009-06-12 09:21:04 PST (-) [reply] If a user edits items while a synchronization is running, then all kinds of "interesting" things can happen: change is overwritten by server change (definitely!), change is made locally but never sent to server (not so sure), ... Part of the problem is that the EDS API is prone to race conditions. This needs to be fixed first, as discussed with the EDS developers: http://www.mail-archive.com/evolution-hackers@gnome.org/msg03029.html Since that discussion I have implemented automatic restore from backup, which has changed my view on the necessary APIs a bit: it would be nice to store items with a specific REV or LAST-MODIFIED if the libebook/libecal user knows what he is doing. Not bumping these fields when restoring an items has the advantage that other clients which were in sync with the restored data don't see any unnecessary changes. The other part of the problem is getting a set of item ID + revision string (REV for contacts, LAST-MODIFIED for event/tasks) in an atomic way. I'm not sure whether the current methods are really atomic. But worse, they are also much slower than they could be because we have to transmit the whole content of the database via D-Bus just to extract the much smaller set of required information. Ross wanted to work on a call to return pairs of UID+REV for libebook. I don't think he got very far with that, though. I think this is a fairly import optimization. ------- Comment #1 From Chen Congwu 2009-08-06 01:16:53 PST (-) [reply] ------- I will look at it. ------- Comment #2 From Chen Congwu 2009-08-09 22:22:42 PST (-) [reply] ------- I have several questions regarding the api implementation. Do we need to impl this concurrent feature for all backends? (groupwise, google, exchange, etc.) When is eds-dbus port ready? Do we need work on this feature on both bonobo and dbus? If so, which one has more priority? [1] http://mail.gnome.org/archives/evolution-hackers/2009-August/msg00004.html (In reply to comment #0) ------- Comment #3 From pohly 2009-08-10 02:17:11 PST (-) [reply] ------- (In reply to comment #2) Doing it for the file backend would be enough. Ross, what is the timeline for merging this into master? I'd say D-Bus is more important, but I don't think upstream would merge this unless we also do Bonobo/CORBA - it remains alive. Ross, do you know what the plans are regarding that? ------- Comment #4 From Ross Burton 2009-08-10 02:28:49 PST (-) [reply] ------- We're merging the addressbook port in the next week or so, but I expect moblin 2.1 to ship eds-dbus still. Please work on eds-dbus first so that we can ship this code. ------- Comment #5 From pohly 2009-08-10 03:14:18 PST (-) [reply] ------- (In reply to comment #4) Is that the "dbus" branch in git.gnome.org? Do you rebase that branch or is it safe to pull from it and create the new API in a feature branch based on the "dbus" branch? ------- Comment #6 From Ross Burton 2009-08-10 06:41:35 PST (-) [reply] ------- At present, and I expect for 2.1 unless magic happens, we're shipping eds-dbus from git.o-hand.com. ------- Comment #7 From Chen Congwu 2009-08-10 18:24:46 PST (-) [reply] ------- So I will work on this repo and provide patch against this. (In reply to comment #6) ------- Comment #8 From Chen Congwu 2009-08-11 18:13:27 PST (-) [reply] ------- After reading the eds-dbus code, I suspect "RETURN-REV" capability can't be implemented for existing non-atomic calls. The reason is: the dbus call does not return "revision" or "Contact", the changed item is asynchronously popped up by another signal ("contacts-changed", etc.). What's worse, the signal has a buffering mechanism, it will not emit as soon as a contact is changed(maybe delayed to a later point to pop up a batch of changes). Two solutions: 1) change the dbus interface (incompatible) 2) Use the workaround which was used by syncevolution in libebook (get the whole contact again by another dbus call, which still can not guarantee the correct semantic (the contacts may already be changed by another client). I suggest not support "RETURN-REV" capability in existing non-atomic calls. For the new added atomic calls, we can use a different dbus interface which will return the "revision" information from the dbus method call. Ross and Patrick, do you think so? ------- Comment #9 From pohly 2009-08-11 23:36:23 PST (-) [reply] ------- (In reply to comment #8) Isn't that interface EDS internal anyway and thus can be changed as necessary? Packagers must be sure to not put incompatible libebook and EDS on the same system, but that has always been the case. That packagers sometimes forgot about the necessary "conflicts" entries in their packages is a different problem... Using different communication paths with EDS to implement the old and new update/delete calls smells like unnecessary code duplication to me. I'd rather extend the protocol so that it works for the more advanced new calls and then map the old ones to the new ones in libebook (they are intentionally strict subsets). ------- Comment #10 From Ross Burton 2009-08-12 00:20:17 PST (-) [reply] ------- I've been telling everyone who wants to talk to EDS via DBus directly not to because the protocol isn't stable, and this is exactly why. We can change the DBus protocol easily, what would the exact changes be? ------- Comment #11 From Chen Congwu 2009-08-12 00:47:52 PST (-) [reply] ------- Thant's good, so I will change the dbus api. The modification apis(create,remove,modify) need be extended to: 1) return the new rev in the dbus call 2) add a new input parameter "flags" to indicate different semantics (ignore rev check, use rev check and do not update rev) Another thing is can I take this assumption also to the backends? That is can I modify existing backends apis instead of adding new apis? Personally I don't think so (The backends api is not internal, there maybe third party backend implementations existed). (In reply to comment #10) ------- Comment #12 From Ross Burton 2009-08-12 02:28:21 PST (-) [reply] ------- The backend API is meant to be somewhat stable, but we've broken it anyway for the DBus branch so if you want to break it more, do it soon! ------- Comment #13 From Chen Congwu 2009-08-21 08:27:40 PST (-) [reply] ------- Created an attachment (id=2325) --> (http://bugs.meego.com/attachment.cgi?id=2325) [details] patch set for this issue Ross & Patrick, this is the patch I provided for this issue, could you please help review? ------------- This patch set aims at solve eds atomic modification problem, see posts in evolution-hackers mailing list. The first 3 patches are api changes. The next 8 patches are: 0004 addressbook libebook (dbus client) new api implementation 0005 addressbook libedata-book (dbus serivce) new api implementation 0006 addressbook file backend new api implementation 0007 addressbook unit test 0008 calendar libecal (dbus client) new api implementation 0009 calendar libedata-cal (dbus service) new api implementaion 0010 calendar file backend new api implementaion 0011 calendar unit test Interface changes and impact: 1. Dbus api has been extended. Applications use dbus directly will break 2. Backend virtual method has been changed. Old backends has to be changed according to work with the new api (Even if it does not provide the new capability) 3. libebook api has a small change (subjet to change): For the async modification apis, "return-rev" capability requires the new revision information be returned to client by the user-supplied callback. Therefore the callback function is changed to add another input parameter. Mutlti-thread problems: Added a multi-thread test for addressbook, see patch 0007. The test fails because of a known bug in libdbus (Phantom OOM). With a patch found in the mailinglist, the test can pass but sometimes client will block a while. Seems this is also caused by libdbus or glib binding. ------- Comment #14 From pohly 2009-10-22 06:29:29 PST (-) [reply] ------- Without this feature, running syncs in the background is risky because of the race conditions. ------- Comment #15 From Chen Congwu 2009-10-24 20:09:43 PST (-) [reply] ------- More from Suyog: (The manual settings you need to set up) ------- Comment #16 From Chen Congwu 2009-10-24 20:11:17 PST (-) [reply] ------- (In reply to comment #15) Sorry, I just copy & pasted to a wrong bug entry... ------- Comment #17 From pohly 2009-11-25 00:56:44 PST (-) [reply] ------- Ross, as I said in our chat, we need to decide how to move forward with this patches because they are needed for automatic sync. Therefore I am raising the priority to P1 temporarily and assign this to you. Next steps: * check as many of the patches as you can * rebase/update remaining patches according to your comments * create bugzilla.gnome.org and request inclusion there (?) ------- Comment #18 From Ross Burton 2009-12-02 04:03:24 PST (-) [reply] ------- As we'll shortly be switched to eds from upstream master, would it be possible to rebase these patches on evolution-data-server from git.gnome.org? ------- Comment #19 From Chen Congwu 2009-12-02 21:44:00 PST (-) [reply] ------- Ok, I will do this, you may have to wait 1~2 days as the patch is a little large. (In reply to comment #18) ------- Comment #20 From Chen Congwu 2009-12-06 01:18:24 PST (-) [reply] ------- (In reply to comment #19) Done, you may pull from http://mob-sync1.sh.intel.com/evolution-data-server.git ------- Comment #21 From Chen Congwu 2010-01-10 22:07:00 PST (-) [reply] ------- (In reply to comment #20) Ross, do you have any plan for reviewing this? -- Configure bugmail: http://bugs.meego.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching someone on the CC list of the bug.