[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: [PATCH] thread-safe KQueuedDialog episode 2
From:       Sebastian Trueg <trueg () kde ! org>
Date:       2008-11-27 16:37:20
Message-ID: 492ECCC0.6060401 () kde ! org
[Download RAW message or body]

we will get there... :)
new patch attached.

Matthias Kretz wrote:
> The changes all look good to me. Though I'm not sure they're enough to make it 
> thread-safe. What about concurrent accesses to KDialogQueue::queue? One thread 
> might be appending while the GUI thread calls pop.
> 
> On Thursday 27 November 2008 15:28:46 Sebastian Trueg wrote:
>> you are of course perfectly right. I have no idea why I did not put that
>> into the constructor. Will do. Other than that, patch ok?
>>
>> Matthias Kretz wrote:
>>> Hi,
>>>
>>> On Thursday 27 November 2008 10:35:42 Sebastian Trueg wrote:
>>>>  KDialogQueue* KDialogQueue::self()
>>>>  {
>>>>    K_GLOBAL_STATIC(KDialogQueue, _self)
>>>> +
>>>> +  // we want KDialogQueue to be thread-safe, i.e. it should be possible
>>>> +  // to queue dialogs from any thread, not only the GUI thread. For
>>>> that +  // to work, the KDialogQueue has to live in the GUI thread. Only
>>>> then +  // the queued connections used below will end up there.
>>>> +  if(_self->thread() != QApplication::instance()->thread() &&
>>>> +     _self->thread() == QThread::currentThread())
>>>> +      _self->moveToThread(QApplication::instance()->thread());
>>>> +
>>> AFAICS you can move this code into the KDialogQueue constructor. That way
>>> you're guaranteed to get a KDialogQueue object that is linked to the GUI
>>> thread from all threads. Otherwise you have a race condition where two
>>> threads access this function, the non-GUI thread creates the object and
>>> the other thread starts to use it before the thread that created the
>>> object moves it to the GUI thread.
>>> Also you then have to do the thread affinity check only once instead of
>>> for every access.
>>>
>>>>    return _self;
>>>>  }
> 

["kdialog-thread-safe-queued-dialogs5.diff" (text/plain)]

Index: dialogs/kdialog.cpp
===================================================================
--- dialogs/kdialog.cpp	(revision 888795)
+++ dialogs/kdialog.cpp	(working copy)
@@ -3,6 +3,7 @@
  *  Additions 1999-2000 by Espen Sand (espen@kde.org)
  *                      by Holger Freyther <freyther@kde.org>
  *            2005-2006 by Olivier Goffart (ogoffart at kde.org)
+ *            2008 by Sebastian Trueg (trueg at kde.org)
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Library General Public
@@ -35,6 +36,10 @@
 #include <QTimer>
 #include <QVBoxLayout>
 #include <QWhatsThis>
+#include <QtCore/QThread>
+#include <QtCore/QMutex>
+#include <QtCore/QMutexLocker>
+#include <QtCore/QQueue>
 
 #include <klocale.h>
 #include <kpushbutton.h>
@@ -1009,9 +1014,9 @@
     void slotShowQueuedDialog();
 
     KDialogQueue *q;
-    QList< QPointer<QDialog> > queue;
+    QQueue< QPointer<QDialog> > queue;
     bool busy;
-
+    QMutex mutex;
 };
 
 KDialogQueue* KDialogQueue::self()
@@ -1023,6 +1028,13 @@
 KDialogQueue::KDialogQueue()
   : d( new Private(this) )
 {
+  // we want KDialogQueue to be thread-safe, i.e. it should be possible
+  // to queue dialogs from any thread, not only the GUI thread. For that
+  // to work, the KDialogQueue has to live in the GUI thread. Only then
+  // the queued connections used below will end up there.
+  if(thread() != QApplication::instance()->thread())
+      moveToThread(QApplication::instance()->thread());
+
   d->busy = false;
 }
 
@@ -1035,9 +1047,17 @@
 void KDialogQueue::queueDialog( QDialog *dialog )
 {
   KDialogQueue *_this = self();
-  _this->d->queue.append( dialog );
+  QMutexLocker lock( &_this->d->mutex );
+  _this->d->queue.enqueue( dialog );
 
-  QTimer::singleShot( 0, _this, SLOT( slotShowQueuedDialog() ) );
+  // move the dialog to the GUI thread
+  if(dialog->thread() != QApplication::instance()->thread() &&
+     dialog->thread() == QThread::currentThread())
+      dialog->moveToThread(QApplication::instance()->thread());
+
+  // We use a queued connection to be able to use queued dialogs
+  // from threads other than the GUI thread (example: KMessageBox)
+  QMetaObject::invokeMethod( _this, "slotShowQueuedDialog", Qt::QueuedConnection );
 }
 
 void KDialogQueue::Private::slotShowQueuedDialog()
@@ -1047,10 +1067,10 @@
 
   QDialog *dialog;
   do {
+    QMutexLocker lock( &mutex );
     if ( queue.isEmpty() )
       return;
-    dialog = queue.first();
-    queue.pop_front();
+    dialog = queue.dequeue();
   } while( !dialog );
 
   busy = true;
@@ -1058,8 +1078,11 @@
   busy = false;
   delete dialog;
 
-  if ( !queue.isEmpty() )
-    QTimer::singleShot( 20, q, SLOT( slotShowQueuedDialog() ) );
+  if ( !queue.isEmpty() ) {
+      // We use a queued connection to be able to use queued dialogs
+      // from threads other than the GUI thread (example: KMessageBox)
+      QMetaObject::invokeMethod( q, "slotShowQueuedDialog", Qt::QueuedConnection );
+  }
 }
 
 #include "kdialog.moc"


[prev in list] [next in list] [prev in thread] [next in thread]