Re: Thread-safe access to graphic objects - proposal

[Michael Goffioul]

As I already mentioned, I proposed a locking scheme a few months
ago. See http://www.nabble.com/Octave-backend-synchronization-td15038411.html
for the complete thread. What I proposed is exactly what I proposed 6 months
ago, except for the boost dependency.

In the thread, you'll see 2 different stand-points. We didn't really reach
an agreement and the thread didn't go further. These 2 options are the
following:
1) implement auto-locking in the get/set functions of the graphics object
2) provide a global locking mechanism in gh_manager that must be used
manually everytime the graphics system is accessed

The patch I proposed implements option 2. John was in favor of option 1,
because it was easier to maintain and less error-prone as the user of
the graphics classes wouldn't have to remember to lock the graphics
system manually (it's done automatically in get/set methods).

While I agree with the fact that an auto-locking system would be easier,
I think option 1 does not solve all problems. Let's imagine you have a
function that "get" the same property at 2 different locations (for instance
at the beginning and at the end): nothing prevents another thread to modify
the property between the 2 "get" calls, which can lead to inconsistencies
in the code using the graphics system. Of course you can say to the
user he has to "get" all properties he needs at the beginning of the
function and only use the obtained values, but I don't think this is better
to remember to use a global lock. Moreover, even if you only use local
copies of property values, this is not completely safe: you can't
guarantee that the value of 2 linked properties will be consistent. For
instance in the following

void myfun (const patch::properties& p)
{
  Matrix v = p.get_vertices ().matrix _value ();
  Matrix x = p.get_xdata ().matrix_value ();
}

nothing prevents v and x to be inconsistent.

So, this is the current status.

Michael.

On Thu, Jul 3, 2008 at 12:22 PM, Maciej Gajewski
<address@hidden> wrote:
> As Michael Goffioul points, copying all properties from main thread to
> GUI thread, and then sending all properties changed by GUI back to
> main thread is hell.
> While just copying them one-way would be not so bad, sending changes
> back from GUI and synchronizing requires complicated and
> hard-to-maintain facilities.
>
> And GUI changes its properties pretty often: each time figure window
> is moved or resized, it's properties should change. Then: axes are
> zoomed, rotated, annotation can be added etc etc.
>
> This all could be made simple bu just adding locks in appropriate
> places in graphics.h /graphics.cc. Having thread-safe property access
> could allow GUI backend to simply read and modify it's properties at
> any time.
>
> Proposed implementation
> ======================
>
> 1. Use boost libraries. they are beautifully designed, crossplatform,
> plays well with std and are widely available.
> 2. Add one central mutex to gh_manager instance.
> 3a. Add lock() / lock_shared() / unlock() / unlock_shared() to gh_manager, or
> 3b. simply leave the mutex exposed, so anyone can lock/unlock it, or
> use locker object to get simple RAII locking.
> 4. Add appropriate locks/unlocks in graphics.cc, in places where:
>  * properties are modified,
>  * new objects are created,
>  * objects are destroyed,
>  * properties are read.
>
> Then - backend living in another thread could use the same locking
> mechanism to access/modify properties.
>
> Another idea is to lock in every property set/get method, but this
> would exclude case where objects are destroyed, possibly leaving GUI
> thread with dangling pointer. So IMO entire blocks of code obtaining
> object pointer from handle, then properties from object, then
> reading/manipulating them.
>
> Using boost RAII classes this could be pretty simple:
>
> void backend::readprops()
> {
>  shared_lock lock( gh_manager::get_mutex() ); // this obtains lock.
> For modification, use unique_lock instead.
>
>  // this is actual part of code from fltk and qt backend
>  graphics_object fobj = gh_manager::get_object (fh);
>  if (fobj &&  fobj.isa ("figure") )
>  {
>    // get properties
>    figure::properties& fp = dynamic_cast<figure::properties&>(
> fobj.get_properties() );
>
>    // read properties here... (or modify, if unique_lock used)
>  }
> } // here the lock dies, releasing mutex
>
> I volunteer to implement this, but I would need help adding
> appropriate checks/dependencies into build system.
>
> RFC
>
> Maciek Gajewski
>