GNOME Bugzilla – Bug 686684
Avoid lost updates by checking the revision of committed contacts.
Last modified: 2013-09-14 16:56:14 UTC
EDS does not implement locks or transactions which makes lost updates possible (cf. write-write conflict). The local file backend stores a database revision number which increases with each database update. When writing an EContact instance the local file backend checks if the contact already got a revision number, and otherwise attaches the global revision number to it. Other than that no further tests are performed. We could avoid the lost update problem by taking the contact's revision number into account when updating the database through the single writer process: We would only accept the update if the contact's REV attribute matches the REV attribute stored for that contact, or if it matches the database's global revision number. In all other cases we would reject the update and report an error. As this change might break existing clients, this behavior would be made optional, possibly by configuring this behavior in an ESourceExtension of the addressbook.
Created attachment 229620 [details] [review] Avoid race conditions, check revision is up to date when committing contacts This patch avoids writing a contact and returns a specialized error whenever a contact revision is out-of-date at commit time.
Created attachment 229621 [details] [review] Test case ensuring that competing threads properly commit contact fields This regression test ensures that there is no conflict when trying to update a contact, many threads race to commit their specific contact field and retry until the each thread succeeds. After all threads finish, the final contact is check that each field was successfully set.
(In reply to comment #0) > As this change might break existing clients, this behavior would be made > optional, possibly by configuring this behavior in an ESourceExtension of > the addressbook. Enforcing it per address book has the advantage that clients don't need to be updated. For example, Evolution would automatically get an error if it submits stale data (which should not happen often, assuming that the contact editing dialog pays attention to contact modification signals). I just wonder what the user experience is if Evolution commits stale data - do we need a dedicated error dialog? Something like "Contact was modified in the meantime, do you a) want to commit your changes anyway and overwrite the other changes or b) drop your changes?". SyncEvolution would detect this particular error and then skip updating the contact in the current sync cycle. In the next sync cycle, it'll apply its merging algorithm to the then-current data and write back the result. This issue is about modify/modified conflicts. There's also - modify/removed (EDS client wants to modify contact which was removed): this should already be handled with an error about "contact not found" - remove/modified (EDS client wants to remove contact which was modified): this is not handled - is there a separate issue about this case? The
I'll summarize my point of view after our chat on IRC. Please correct where I'm wrong. First of all, the RFC [1] doesn't say who is responsible for setting REV on a contact. In eds/evo world was decided to left this responsibility up to the backend. It works fine, as the backend is a centralized point which takes care of the storage. The thing is that not every backend is able to do that properly, because groupware backends, like for Exchange or Groupwise, are just "proxies", and the remote server should take care of conflicts. Due to this the question is how to compare the REV value. The other question is how to left synchronization software work, like syncevolution, which can sync contacts from a phone device to a local book backend. The backend should not reject rewrite of a contact which is newer on the device, even when the device application changed the REV. Note that you cannot revert REV to local value just to be able to write it to eds, because the sync back to the device will upload the contact there again, even if it didn't change. Unless the synchronization software has some expensive state data store locally. Of course, having dependency on the state data is also suboptimal, like if you lost this data during update for some reason, and you'll re-try to synchronize your phone to book which is already full of the contacts from the phone. Patrick should be able clarify better whether it's a problem or isn't for syncevolution. That said, I want this to be able to cover both use cases: a) detect out-of-sync modifications (it might be useful even for remove operation, though there is not used removed contact itself in eds' API) b) be able to synchronize with 3rd party device/application, where it's expected to store unmodified contact to eds and that the device/application can change REVs on its own (which is expected) The Exchange Web Services interface uses change keys for this purpose, which is set on each objects, like on both folders and items in them, and certain operations require this change key to match with that on the server. How the conflict is handled is up to the client. There is only one difference, the key is their own, thus they are able to work with it without changing actual content of the vCard (or the REV attribute, even they do not use vCard for their storage at all). Just an out of head example. [1] http://tools.ietf.org/html/rfc6350#section-6.7.4 the previous RFC 2425 defines different format of the REV value
Review of attachment 229620 [details] [review]: I would call the error E_DATA_BOOK_STATUS_OUT_OF_SYNC, and define its counterpart as an EClient error (common to both book and calendar) in libedataserver/e-client.h/.c, and then make sure the status is properly restored on the client side, this time in e-book-client.c at unwrap_dbus_error. I've no more comment for this patch currently.
Review of attachment 229621 [details] [review]: Use g_error_matches(), and honestly, I'm quite surprised it works for you, because the EBookClient is supposed to return either EBookClient or EClient errors. I guess once you update the unwrap_gdbus_error() then it'll start to work.
gcc complains about E_CLIENT_ERROR_OUT_OF_SYNC not handled in e_client_error_to_string() diff --git a/libedataserver/e-client.c b/libedataserver/e-client.c index 22b9ab8..bcacaff 100644 --- a/libedataserver/e-client.c +++ b/libedataserver/e-client.c @@ -160,6 +160,8 @@ e_client_error_to_string (EClientError code) return _("Other error"); case E_CLIENT_ERROR_NOT_OPENED: return _("Backend is not opened yet"); + case E_CLIENT_ERROR_OUT_OF_SYNC: + return _("Object is out sync"); } return _("Unknown error");
(In reply to comment #7) > gcc complains about E_CLIENT_ERROR_OUT_OF_SYNC not handled in > e_client_error_to_string() > > > diff --git a/libedataserver/e-client.c b/libedataserver/e-client.c > index 22b9ab8..bcacaff 100644 > --- a/libedataserver/e-client.c > +++ b/libedataserver/e-client.c > @@ -160,6 +160,8 @@ e_client_error_to_string (EClientError code) > return _("Other error"); > case E_CLIENT_ERROR_NOT_OPENED: > return _("Backend is not opened yet"); > + case E_CLIENT_ERROR_OUT_OF_SYNC: > + return _("Object is out sync"); s/is out sync/is out of sync/
Tristan, please incorporate the change from comment #7 and comment #8, when you'll update the patches. Thanks.
Created attachment 234277 [details] [review] Patch adding the E_CLIENT_ERROR_OUT_OF_SYNC and EDataBook counterparts This patch takes care of adding the new error codes, along with wrapping and unwrapping them over D-Bus.
Created attachment 234278 [details] [review] Add ESourceRevisionGuards extension This extension indicates whether checking for revision conflicts is desired. Allowing the implementation to be optional (and OFF by default, avoiding any unexpected error conditions in existing code).
Created attachment 234279 [details] [review] EBookBackendFile: Handle revision conflicts, if revision guards are enabled This new version of the patch uses the new E_DATA_BOOK_STATUS_OUT_OF_SYNC error as requested (which now wraps/unwraps properly), as well as only optionally erroring out depending on the configuration of the revision guards extension.
Created attachment 234280 [details] [review] Test case ensuring that competing threads properly commit contact fields (rev 2) This new version of the test case checks for the E_CLIENT_ERROR_OUT_OF_SYNC error and configures the revision guards to be enabled. Furthermore, it's been ported to use the isolated test framework.
I've updated the patch set in the hope that we can find some middle ground and that hopefully this patch will give us some added stability at least for the local file backend without being too intrusive. While we realize this approach does not take into account all of the possible points of failure (and recovery methods) discussed in this thread[0], we do feel that this is a good simple approach for dealing with the local file backend, at least until a more complex transactional system can be engineered to handle object modification conflicts more gracefully for all backends. So perhaps this can be considered as a stop-gap solution until something better can be created. [0]: https://mail.gnome.org/archives/evolution-hackers/2012-December/msg00001.html
Review of attachment 234277 [details] [review]: Looks good, please commit
Review of attachment 234278 [details] [review]: Let's call the property "enabled"/"Enabled", and the functions ..._get_enabled()/..._set_enabled(). It's always better when the function matches property name. Otherwise no objection (by the way, it's 2013 now) :) ::: libedataserver/e-source-revision-guards.h @@ +83,3 @@ +}; + +GType e_source_revision_guards_get_type (void) G_GNUC_CONST; Rather no G_GNUC_CONST
Review of attachment 234279 [details] [review]: Hmm, you removed the lock around bf->priv->revision variable. I'm not sure if the RW lock will do the same job. Please see why it was added. With your patch, for example, the e_book_backend_file_load_revision accesses the varialbe without having a lock, which can lead to the crash for which the lock was added.
Review of attachment 234280 [details] [review]: Apart of the two nitpicks looks good, feel free to commit when the other patches are in. ::: tests/libebook/client/test-client-write-write.c @@ +26,3 @@ +} + + extra empty line @@ +72,3 @@ +static gboolean try_write_field_thread_idle (ThreadData *data); + + extra empty line
By the way, from the UI part, who will decide whether to use the guards or not? The backend can be used by many clients, and if one decides to use it, but another is not ready for it, then they might fight on it, right?
(In reply to comment #19) > By the way, from the UI part, who will decide whether to use the guards or not? It would be the distro which enables that by tweaking the default. > The backend can be used by many clients, and if one decides to use it, but > another is not ready for it, then they might fight on it, right? Right. Therefore, for "normal" Linux distros where the feature would be off, I would like to see an additional way of enabling it per app. Tristan, do you see a chance to add that as a flag to the EClientBook? If the flag is true (not the default), the app signals to EDS that it wants the revision to be checked even if the source is not configured to do it. If the flag is false, checking is done according to the source settings.
Patrick, I don't see any chance of that without major changes to the APIs EBookClientView has some flags, which can modify how the server deals with a given view. This is possible because the server actually exports a separate D-Bus object for each EDataBookView (and so there is some separate server-side data for each view). EBookClient on the other hand is a proxy to the EDataBook server side object representing a given addressbook. While there can be many proxies connected to the same EDataBook, on the server side there is only one book and no distinction between client connections (and so there is no logical place to store such a flag on the server side). Not to mention, even if there was such a separation, the EDataBook delegates the actions to the EBookBackend implementations, so those EBookBackend APIs would also need to be changed to support such a flag.
Created attachment 234369 [details] [review] Add ESourceRevisionGuards extension (rev 2) New patch uses 'enabled' as property name and pushes the copyright notice to 2013 (actually it was originally part of ESourceAddressBookConfig from 2012 ;-))
Created attachment 234370 [details] [review] EBookBackendFile: Handle revision conflicts, if revision guards are enabled New patch for EBookBackendFile a.) Use changed api name e_source_revision_guards_get_enabled() b.) Use the RWLock to protect the revision string, even at load time c.) Reduce diffs by not creating an extra e_book_backend_bump_revision_locked() function. I doubt that 'b' is really meaningful, as I don't think concurrency is allowed during e_book_backend_file_open(), but better to err on the side of caution.
Created attachment 234372 [details] [review] Test case ensuring that competing threads properly commit contact fields (rev 3) Test case updated to remove some whitespace, and use the new e_source_revision_guards_set_enabled() API to create the test addressbook.
Created attachment 234373 [details] [review] Add ESourceRevisionGuards extension (rev 3) oops, sorry for extra bugmail... changed the copyright to 2013 in *both* the header and source file... details... sigh.
Created attachment 234374 [details] [review] Test case ensuring that competing threads properly commit contact fields (rev 4) Again sorry for extra bugmail... adding missing copyright header to test case.
(In reply to comment #19) > By the way, from the UI part, who will decide whether to use the guards or not? > The backend can be used by many clients, and if one decides to use it, but > another is not ready for it, then they might fight on it, right? FWIW, this is the same question that was raised for EBookBackendSummarySetup, and the question is valid for most if not all ESourceExtension apis. IMO, it does not make sense for clients to be modifying the extensions, except when those clients are to be the 'owner' or 'creator' of the addressbook data. A similar question is what happens when I run the following code: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ESource *source; ESourceBackend *backend; source = e_source_registry_ref_default_address_book (registry); backend = e_source_get_extension (source, E_SOURCE_EXTENSION_ADDRESS_BOOK); e_source_backend_set_backend_name (backend, "webdav"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Do the contacts get pushed to a new webdav backend ? I doubt it, because it just doesn't make sense for multiple clients to be configuring the behaviour of an ESource, it only makes sense when defining/owning a new ESource. That said, it might be interesting to have a similar EClientExtension API, where one could define the preferences of how a client interacts with a backend (without needing to always extend the public EBookClient apis). Of course, as I mentioned in my reply to Patrick, this would require that the server maintain some per-client data representing their preferences.
Review of attachment 234370 [details] [review]: Apart of the note looks fine, feel free to commit. ::: addressbook/backends/file/e-book-backend-file.c @@ -1502,2 @@ *prop_value = g_strdup (bf->priv->revision); - g_rec_mutex_unlock (&bf->priv->revision_mutex); I'm pretty sure you should left here the locking, same as in e_book_backend_file_open() - the lock was added to guard priv->revision, which, unguarded, could cause a crash. See bug #690151.
Review of attachment 234373 [details] [review]: Looks fine, please commit. I use to do the bool check if (extension->priv->enabled == enabled) rather like if ((extension->priv->enabled ? 1: 0) == (enabled ? 1 : 0)) which is not dependant on the actual gboolean (gint) value. I saw on various places that a gboolean variable is constructed as bool |= number; which causes it not to be TRUE/FALSE exclusively.
Review of attachment 234374 [details] [review]: Please fix that one thing and feel free to commit to master. Thanks. ::: tests/libebook/client/test-client-write-write.c @@ +23,3 @@ +#include <locale.h> +#include <libebook/libebook.h> +#include <libedata-book/libedata-book.h> You should not need this, same as its counter-part in Makefile.am, it's meant to be used only by the server side, not by the client side.
Comment on attachment 234277 [details] [review] Patch adding the E_CLIENT_ERROR_OUT_OF_SYNC and EDataBook counterparts Committed and pushed.
Comment on attachment 234370 [details] [review] EBookBackendFile: Handle revision conflicts, if revision guards are enabled Noted and fixed, actually the source in master seems to have changed slightly. Committed and pushed a patch which ensures the ->revision string is protected at any time it's accessed.
Comment on attachment 234373 [details] [review] Add ESourceRevisionGuards extension (rev 3) Committed and protected the truth value. Actually used a different technique which I've seen in other (clutter) sources and find elegant. Added a line in set_enabled(): enabled = !!enabled;
Comment on attachment 234374 [details] [review] Test case ensuring that competing threads properly commit contact fields (rev 4) Removed the inclusion of edatabook and explicit unneeded inclusion of libebook (as well as the added linkage). Actually those are artifacts from the still uncommitted Direct Read Access patches.
(In reply to comment #33) > enabled = !!enabled; I do not know how different compilers operate with such expression, but I can imagine that some of them can optimize this "on the first look NOP" out, when certain optimization level is turned on.
(In reply to comment #35) > (In reply to comment #33) > > enabled = !!enabled; > > I do not know how different compilers operate with such expression, but I can > imagine that some of them can optimize this "on the first look NOP" out, when > certain optimization level is turned on. That would be a bug which should be found fairly quickly, because the idiom is used quite a bit to turn arbitrary values into real booleans with just 0/1 (in C) or false/true (in C++).