[Development] invokeMethod() with function pointers

[Grégoire Barbier]

Le 20/01/2017 à 11:14, Olivier Goffart a écrit :
> On Dienstag, 17. Januar 2017 11:21:56 CET Grégoire Barbier wrote:
>> Le 16/01/2017 à 10:34, Olivier Goffart a écrit :
>> > What's the use case for this function? For direct call you better of
>> > calling the function directly,  and the equivalent of QueuedConnection
>> > can be achieved with QTimer::singleShot.
>>
>> Hi.
>>
>> AFAIK there is no other way to call a method across threads *and wait
>> for it result* than QMetaObject::invokeMethod() with
>> Qt::BlockingQueuedConnection and Q_RETURN_ARG (apart, of course, making
>> the called method thread-safe).
>
> That's a valid use case. Thank you for bringing that up.
>
> [...]
>
>> Also (I still dream), if there was a way to make invokeMethod()
>> automagically choose between a direct call and
>> Qt::BlockingQueuedConnection, it would be possible to get rid of this idiom:
>> if (QThread::currentThread() == this->thread())
>>         foo = func();
>>      else
>>         QMetaObject::invokeMethod(this, "func",
>>                   Qt::BlockingQueuedConnection,
>>                   Q_RETURN_ARG(foo));
>>
>> Kind of Qt::DirectOrBlockingQueuedConnection.
>
> See the discussion in https://codereview.qt-project.org/83404/ for why this is
> not a good idea.
> In summary, BlockingQueuedConnection is dangerous as it can lead to deadlock
> if the other thread is waiting on your thread. In order to use it properly,
> you really need to know, while programming in which thread you are and which
> thread you try to communicate to, and be sure that there is never in your
> program cases where the other thread may be waiting on a QWaitCondition for
> your other thread. So this pattern with QThread::currentThread() == this-
>>thread() before a BlockingQueuedConnection is really dangerous.

Thank you it's very interesting.

But, I use this pattern mainly to protect data-access methods (such as a 
getter on an implicitly shared object) than can be either called from 
the owner thread or another one.
I'll keep using it, without fear, unless the called object's owner 
thread was likelly to change.

I respect the fact that you rejected Qt::BlockingAutoConnection in 2014 
because of its potential danger, but I'm not sure that it's better to 
let people use the "QThread::currentThread() == this" pattern without 
being warned rather than implementing Qt::BlockingAutoConnection, with a 
detailed warning in the doc.

Moreover, I don't understand whi BlockingAutoConnection would be more 
dangerous (if it were to be added) than BlockingQueuedConnection is 
currently already dangerous.

With your own words from 2014:

"""
I would not even try to solve this race with moveToThead, as a mention, 
i think we just need to document that one must not call moveToThread 
when Blocking connecitona re involved.
"""

Maybe it would be interesting to set this comment in enum 
Qt::ConnectionType doc, just right to Qt::BlockingQueuedConnection (or a 
would-be Qt::BlockingAutoConnection) and not only in the "Signals and 
Slots Across Threads" paragraph, to make everything safer with noobs 
(like me) that do not read the whole docs but only spam the F1 key from 
with Qt Creator.

> On the other hand, it would be useful to get the return value of a
> QueuedConnection in a QFuture.

Well.
In french there is a very funny expression which means more or less "to 
use a hammer to kill a fly". ;-)
Let's take an example, I clearly don't think I would like to use a 
QFuture, make it thread-safe with a QMutex and the like in the following 
code (even though you've just make me learn that 
Qt::BlockingQueuedConnection may be dangerous, in the following code I 
know it's not, because A object never changes thread):

class B;
class A : public QObject {
   Q_OBJECT
   QLinkedList<B> _queue;
public:
   /** This method is thread-safe. */
   B pop() const {
     B value;
     if (QThread::currentThread() == this->thread())
       value = doPop();
     else
       QMetaObject::invokeMethod(this, "doFoo",
              Qt::BlockingQueuedConnection,
              Q_RETURN_ARG(B, value));
     return value;
   }
private:
   /** This method is not thread-safe. */
   Q_INVOKABLE B doPop() {
     return _queue.takeFirst();
   }
};

In the other hand, I would by far prefer to write this code, both the 
lambda and Qt::AutoBlockingConnection make the code shorter and easier 
to understand (and I don't expect that using QFuture would lead to the 
same result):

class B;
class A : public QObject {
   Q_OBJECT
   QLinkedList<B> _queue;
public:
   /** This method is thread-safe. */
   B pop() const {
      B value;
      QMetaObject::invokeMethod(this, [&value]() {
              value = _queue.takeFirst();
          }, Qt::AutoBlockingConnection);
      }
   }
};

Of course in this simple case, it's obvious that make the method 
thread-safe (e.g. with a QMutex/QMutexLocker) is easier and lighter than 
a queued call. But it's not always so obvious.

-- 
Grégoire Barbier :: g à g76r.eu :: +33 6 21 35 73 49