Re: Passing variables up to the GUI

[Michael Goffioul]

On Sun, Apr 14, 2013 at 12:54 AM, Daniel J Sebald <address@hidden> wrote:

On 04/13/2013 06:49 PM, Michael Goffioul wrote:

On Sat, Apr 13, 2013 at 3:29 PM, Daniel J Sebald <address@hidden

<mailto:address@hidden>> wrote:

    Well, let's think about this.  How is this going to crash from a too
    deep reference count?  Would it be the case that if the worker
    thread (Octave core) keeps emitting signals while the GUI thread
    doesn't respond to them?  Is that the bad scenario you are describing?


The problem is very basic: 2 (or more) threads trying to access/modify
the reference count of an object concurrently. The copy constructor of
octave_value does:

     rep->count++;

The destructor of octave_value does:

     if (--rep->count == 0)
         delete rep;

Those operations are *not* atomic, this means that while one thread is
incrementing the counter, another one is decrementing it. The result is
undefined. The best case scenario, the counter never reaches 0 and you
get a memory leak. Worst case scenario, you try to access an object that
has been deleted and you get a crash.

There's not much you can do to prevent this, except making the operation
(increment/decrement) on the counter atomic at the CPU level. This is
typically what shared_ptr<> implementation does.

Another ad-hoc solution would be to have the emitter thread to keep a
reference of the passed object, until it is sure that the receiver
thread has finished with it (for instance by receiving a signal back).
But even then, you might get into trouble, as you don't know where and
when all existing references will be deleted: a queued signal must keep
a reference somewhere to the passed objects, which get discarded at some
point, but is it in the emitter thread or the receiver thread?

    There is this option Qt::BlockingQueuedConnection that we could use.
    This runs the risk of, as you said, deadlocking the threads.  But
    that might be fine in this case so long as there is no ->exec() in
    the GUI event loop.  If the deadlocking is placed at moments where
    the GUI just accepts the data and stores it somewhere, builds a
    table, etc., I don't think that is too critical.  If just the
    connections that are requesting octave_value_list data are
    BlockingQueued, then these are the GUI routines that are just
    wanting some data and not much else.  I'm not sure using
    BlockingQueued is a good idea though in a complex system; not
    without some thought anyway.  There is too much risk of some other
    programmer putting in an ->exec () into the code not realizing that
    is bad.


Blocking calls in threads is probably a bad practice imo.

I agree with that.  It would be the last resort, but I don't think it is necessary.

Can't we avoid this issue just be not creating or deleting octave_value objects?  

The problem  is that you don't control all of them.

 

Here's the scenario:

In the worker thread is the signal:

   retval = eval_string (first_command->toStdString (), false, parse_status, first_nout);
   if (!parse_status && !error_state)
     {
       emit octave_command_result (first_qobject, retval);
     }

(could possible include the command string along with the result as a reminder).  Now, if the signal were direct (which it isn't), then Qt wouldn't have to do anything special.  The call by reference could probably break down to a pointer-like operation.  However, if Qt queues the signal (which is the case) then it has to do something to avoid possible loss of that data in the thread.  It will have to copy the data somehow (if it were a pointer instead of reference, then that's a different story); at least that is my guess--and it doesn't have any knowledge about the constructor required to copy the object representation (which constructor would it call?) so it must just do a memcpy of the structural representation of the object (but yes, if there is a member variable that is pointing to a static value, that's potential problems).

I don't think that's true. When using queue connection, Qt will make a copy of the arguments using the copy constructor, that's part of the qRegisterMetaType template stuff. And a copy constructor is a requirement for using qRegisterMetaType. Moreover, if you pass an object by reference, it'll be treated as pass-by-value if you send it across threads [1].

 

OK, now in the GUI thread I did something like this:

[snip]::handle_octave_command_result (const QObject* ID, const octave_value_list& ovlres)
{
[snip]
      octave_value octval = ovlres (0);
      if (octval.is_numeric_type ())
        {
          Matrix matval = octval.matrix_value ();
[snip]

The rest of the code just accesses the data in matval.  So, if I understand you correctly, the dangerous operation (provided an optimizing compiler doesn't just remove it) is this creation of the octave_value on the stack:

There's more than that. ovlres will be copied by Qt on queued connection. In the above code, octave_value_list, octave_value and Matrix uses reference counting. So any operation on these is dangerous.

 

      octave_value octval = ovlres (0);

(even then I'm wondering if there is an actual constructor/destructor).  That can be avoided though, with the following:

[snip]
      if (ovlres(0).is_numeric_type ())
        {
          Matrix matval = ovlres(0).matrix_value ();
[snip]

Then certainly no constructor/destructor is called.

The default constructor of Matrix is called. The operator= of Matrix is called, which changes its refcount. When matval gets out of scope, its destructor will be called, decrementing its refcount. When the slot completes, the internally-copied ovlres will also get destructed.

We can argue as long as we want about this, it doesn't change the fact that there's a fundamental concurrency issue on the reference counting: it is not thread-safe. This can be avoided by compiling octave with --enable-atomic-refcount.

Michael.

[1] http://comments.gmane.org/gmane.comp.lib.qt.general/14359