Re: [Evolution-hackers] improve PHOTO support in libebook/EDS

[Tristan Van Berkom <tristanvb openismus com>]

On Wed, 2011-07-06 at 18:15 +0200, Patrick Ohly wrote:
> Hello!
> 
> It was pointed out before that support for PHOTO data in EDS is
> sub-optimal because the data has to be encoded as B64 data in the vCard
> and then gets transmitted like that via D-BUs and stored in Berkley DB.

Hi,
   Unfortunately I started this off list so I'm just going to append
my original off-list proposal at the end of this mail (it may help
for people to understand the context better).


> 
> A high-level description of the problem is here:
> http://wiki.meego.com/Architecture/planning/evolution-data-server/eds-improvements#Contacts:_Store_PHOTO_Data_as_Plain_Files
> --------------------------------
> Photos should be transferred over D-Bus via a filepath to a local file
> rather than transferring the actual photo data over D-Bus. The VCard
> format already allows this option (See the PHOTO property). We must be
> careful to keep these local files in sync and to remove them when
> contacts are removed, and to restrict access to the files from other
> processes. 
> 
> There should be utility code available to libebook users to convert
> between the different representations.
> 
> Goals:
> 
>      1. Apps should be able to create photo files without ever passing
>         the data to libebook.
>      2. External PHOTO file and corresponding contact must be kept in
>         sync: 
>              1. If a contact is deleted, the photo also needs to be
>                 removed.
>              2. If a photo file is created and then storing the contact
>                 fails, who removes the file?
>      3. Access to photo data must be limited to processes which have the
>         right permissions.
> -------------------------------
> 
> There also was a proposal for how to solve this, but I am no longer
> entirely happy with it.
> 
> Openismus is working on this at the moment, see
> https://bugzilla.gnome.org/show_bug.cgi?id=652178
> 
> Before diving further into prototyping or implementing a solution, I
> would like to come to an agreement about the client visible behavior and
> a rough outline how to implement it.
> 
> First let me clarify that we need to distinguish two different use
> cases:
>      1. The photo app owns and manages a set of photos. A link is added
>         to a contact. If the contact is deleted, the photo must remain
>         because it is part of a slide show. If the photo is moved or
>         renamed, the link remains but is obviously no longer useful
>         ("dangling link" as in a symbolic link in a file system).
>      2. Photo data is attached to a contact. EDS becomes the owner for
>         the data and must ensure that it is stored and deleted together
>         with the contact. Apps get access to the photo via its URI,
>         exactly as in the first case.
> 
> The extension that we are discussing addresses the second point. It must
> not break the first one.
> 
> Tristan's plan seems to be to pass the PHOTO data inline in the vCard to the
> file backend and to strip it out there. Note that this is not optimal in
> terms of performance: if the data is already available separately (as it
> is in syncing, for example), then it has to be inlined and transferred
> via D-Bus (slow). This is acceptable because contacts are read a lot
> more often than written. But that doesn't stop people from running write
> benchmarks ;-)
> 
> I'd only go down that route if it really makes the implementation a lot
> simpler. From the description in 652178 and private email it doesn't
> sound like it necessarily does.
> 
> How about a different approach:
>       * Each EBookClient can request a "staging" directory. If the
>         backend supports this mechanism, it is created for this client
>         by EDS. If not, the client has to inline PHOTO data as before.
>       * The content of the staging directory is ephemeral. When a client
>         crashes or disconnects, EDS removes the directory and everything
>         inside it. When EDS crashes, it cleans up all staging
>         directories when it restarts.
>       * If a client wants EDS to take ownership of the photo, it creates
>         a file in the staging directory with an arbitrary name and sets
>         the PHOTO URI such that it points to it with an absolute path.
>       * The client submits the contact. At that point it stops being
>         responsible for the file.
>       * The EDS backend checks for such special PHOTO URIs. If it finds
>         one, it copies or moves the file to its permanent location and
>         updates the URI before storing the contact and notifying
>         clients. Note that backends are free to do that however they
>         want. For example, a CardDAV backend might upload the file data
>         to a resource on the HTTP server and set the PHOTO URI to a
>         http:// URL. The file backend chooses a staging directory in the
>         same file system as the permanent photo directory and thus can
>         simply move the file instead of having to copy it.
> 
> libebook might offer some additional utility code to simplify the
> handling of such a staging directory and PHOTO URIs. API to be decided,
> also for the "request a staging directory" part.

This alternative proposal is strictly regarding the photo data for
which the server must take ownership of I assume, correct ?

(for instance, we don't want to duplicate photos from the photo
reel into the staging directory and then into a new permanent
location owned by the addressbook).

I might not have been clear enough in my proposal but I only 
intend for new or modified photos to be sent to the addressbook.

In other words most of the time you receive a contact with a uri
owned by the addressbook, you are allowed to send those same uris
back to the addressbook in calls to e_book_commit_contact() but 
only need to send blobs of photo data when the photo data itself 
should be changed.

The staging directory you propose if I understand it correctly
would also be used under only the same circumstances, I suppose
the questions are:
  o What is faster: writing a file to a staging directory and 
    then moving it to a permanent location or sending the blob
    once over D-Bus and saving it to the permanent location.
  o What is more reliable ? (it seems that there are less
    points of failure when sending the blob but I could be wrong).

Perhaps the staging directory is a little faster when batching lots 
of photo modifications during a 'sync' operation ? (the client
gets to write to disk while waiting for the server to be ready
for the next batch of contact additions perhaps ?)

At any rate, if it's judged that the performance gain of using
a staging directory is worth the added complexity then let's do it.


I think the more important issue at hand is that to offer
a reliable api for EBookView then the backend has to track
the views which have received notification of the contact
modifications.

In other words when I have an EBookView and I have been
notified that a contact exists with a photo at a given uri path,
then until I receive notification that that uri is invalid,
removed or replaced with a new one... I should be able to
access that file.

So there is now a race condition at the moment one process
removes a contact that another is viewing. This condition
was not an issue before because one could assume that
a vCard you are dealt is not fragmented (you have the
whole vCard in one chunk until you are told otherwise,
completely safely and asynchronously).

Another hard limitation is regarding storing uris for which
you don't want the addressbook to assume ownership of.

The clean way as far as I can see is whoever is responsible
for adding an 'alien uri' to an addressbook is responsible
for removing that entry from the addressbook at the time
that the uri might be deleted.

So either:

   o PhotoApp has shortcut to assign photo from reel to contact and
     is responsible for blocking on the return of
     e_book_contact_modify_async() before removing the files from disk

   o MyContactAvatarApp asks PhotoApp for a resource in the reel
     and claims it with PhotoAppClaimUri(), then proceeds to add that
     uri to the addressbook... Later MyContactAvatarApp receives
     notification from the PhotoApp that the uri claimed with
     "PhotoAppClaimUri()" is being deleted, removes and waits
     on the addressbook to return from e_book_commit_contact_async()
     and then does PhotoAppUnClaimUri() at which point the photo app
     safely removes the data.

   o MyContactAvatarApp duplicates the data from the photo reel and
     acts responsibly in the place of PhotoApp at the cost of duplicated
     avatars on disk.

In other words the provider of an external 'alien uri' has the same
responsibility as the addressbook backend assumes for the uris it
manages.

Well... perhaps we should just swallow the ugly race and do nothing
about it and say that:
  "a uri might be invalid from time to time directly before your view
   receives notification of the removal" ?

Would that be acceptable ?


Best Regards,
      -Tristan

This was my rather elaborate off-list proposal:
> 
> I have a plan in action to fix the gdbus issues which I outlined
> in the bug report[0], here is a better detailed plan.
> 
>   o A photo field with a user provided uri is provided and managed
>     presumably by the user who added the contact, EDS will not try
>     to access the data in any way.
> 
>     So uris provided by users are expected to be managed by
>     users, for this to work reliably; libebook needs to be fixed
>     to more properly implement the functions:
> 
>       e_book_add_contact_async();
>       e_book_commit_contact_async();
>       e_book_remove_contact_async();
>     (i.e. they need to wait for the views to be actually notified)
> 
>     These above methods need to be called by applications providing
>     uris for contacts (such as the photo application which might link
>     a user's contact to an item in the photo reel)... when the photo
app
>     needs to delete a photo that is connected to an avatar it should
>     wait for the async reply of e_book_commit_contact_async() (this
will
>     ensure that any other running applications with a reference to
that
>     resource are notified before the photo app goes to delete the
>     photo).
> 
>   o Photo fields that are inputted as binary data will be dumped to
>     disk, stored as a uri and fall under the responsibility of the
>     addressbook.
> 
>     Like any client who provides a uri, the addressbook is responsible
>     for the validity of the uris it manages (that is to say the uri
can
>     always be accessed by a client until the client receives a
>     notification that the uri is gone).
> 
>   o Now the file backend will store photos along side the BDB:
> 
>     $(XDG_DATA_HOME)/evolution/addressbook/$(ESource token)/db/
>     $(XDG_DATA_HOME)/evolution/addressbook/$(ESource token)/photos/
>     (I've moved the DBD into the 'db/' subdir for this to look right).
> 
> And here's a run down of the effected modules inside the addressbook
> code:
> 
>   o EDataBookView
> 
>     This is a server side representation of an EBookView, any chatter
>     between the backend and the client on the server side goes through
>     this module (it sends and receives messages using EGdbusBookView).
> 
>     Essentially the tricky part that needs to be done here is reply
>     callbacks need to be delivered to the callers of
>     e_data_book_view_notify_update() (so that the backend can know if
>     and when a client received the contact modification).
> 
>     This will allow for a proper implementation
>     e_book_commit_contact_async() as well as allow the backend 
>     to listen to the notifications.
> 
>   o EBookView
> 
>     As mentioned the sync/async versions of e_book_*() apis that
modify
>     contacts need to be implemented to wait on views (perhaps this
does
>     not require any code changes to libebook though, even).
> 
>   o EGdbusBookView
> 
>     Here we have a tricky problem, unfortunately the dbus
>     specification[1] imposes on us that an exported object on the bus
>     cannot wait on any message reply from a proxy, and without a
message
>     reply to contact notifications we cannot reliably implement this.
> 
>     So my plan is to create an EGdbusBookViewReceiver object.
> 
>     That will leave us with an EGdbusBookView object exported
>     on the bus on behalf of the EDataBookView on the server (as usual)
>     and now an EGdbusBookViewReceiver object exported on the bus on
>     behalf of an EBookView on the client.
> 
>     The 'ContactsAdded', 'ContactsRemoved', 'ContactsChanged', and
>     'complete' dbus signals on EGdbusBookView will be removed in
favour
>     of dbus methods on the new EGdbusBookViewReceiver.
> 
>     Calls which notify the client about contact changes will always be
>     made asynchronously on the receiver object belonging to the
client.
> 
>   o EBookBackendFile:
> 
>     It's worth mentioning that EBookBackendFile is derived from
>     EBookBackendSync (and EBookBackend) which is the abstraction 
>     used by EDataBook (you can say that the business logic of server
>     is done as much as possible in EDataBook and EDataBookView and
>     everything backend specific is run by an EBookBackend delegate
>     object owned by the EDataBook).
> 
>     That being said the plans here in the local file backend are: 
>       o Transform incomming binary photo blobs into uris owned by 
>         the server rather than storing any binary photo blobs in 
>         the BDB
>       o Assume responsibility for deleting files on disk which 
>         it created and ensure that the file is always on disk until
>         the last view has received notification of it's removal.
> 
>     Luckily, the EBookBackend delegate object is responsible for
>     notifying all views after adding, modifying or removing any 
>     contacts (this at least gives us a good opportunity to wait 
>     for client views to receive notifications).
> 
> [0]: http://bugzilla.gnome.org/show_bug.cgi?id=652178
> [1]: http://dbus.freedesktop.org/doc/dbus-specification.html 
>      (See "Message Types")