[Qt-interest] AmIRight: Can't Have Model and View on different Threads?

[Stephen Kelly]

Hostile Fork wrote:

> Hello all...!

Hi,


> 
> I'm on day #2 of looking at Qt's Model/View architecture, and had a
> question that turned into an essay.  By the end of it, I figured I'd
> most likely answered what I started with... but I thought I'd run it
> by qt-interest (for criticisms if I'm wrong, and for Internet-
> Searching-Posterity if I'm right!)
> 
> The short version is that I don't think it's feasible for a Model to
> be modified on a non-GUI thread...regardless of whether the model's
> data has been protected with read/write locks.   If what I'm gathering
> is correct, then Qt should probably have an assert that a model and
> its view have the same thread affinity (it doesn't seem to do that now)

I don't know much about threading stuff yet, but you explanation for this 
looks fairly reasonable too me.

>From my reading of it, the signal/slot connections between the model and 
view have to be made with Qt::DirectConnection. However, making connections 
made across threads would have to be Qt::QueuedConnection.

http://doc.trolltech.com/4.5/threads.html#signals-and-slots-across-threads

So, if your model was in a different thread to your gui, there could come a 
point where there is a queue containing beginInsertRows(...), 
endInsertRows(), beginRemoveRows(...), endRemoveRows() if two threads had 
operated on it as you wrote. Then when the view tries to get the data in 
response to the endInsertRows signal, the data would already be gone.

So, I think you're right, the model, all intermediate proxies and views all 
have to be in the same thread.

However, I think you can make the model use worker threads and then the 
model ensures that the correct signals are emitted, directly, and in the 
correct order. I think you would still have to use locks to ensure that a 
emailsRemoved signal from a worker thread doesn't get processed by the model 
until the correct time.

Incidentally, I've written a model for handling emails and other PIM data 
where all the work is done in different *processes*. Notifications are 
delivered by a Monitor class and processed by the model as soon as they are 
recieved. Changes made by the model (from the user) are initiated via Jobs, 
the results of which also come back through the Monitor. See in particular 
the private slots:

http://websvn.kde.org/trunk/KDE/kdepimlibs/akonadi/entitytreemodel.h?view=markup

More dox here, which I've just realised I have to move:

http://api.kde.org/4.x-api/kdepim-
apidocs/akonadi/akonadi_next/html/index.html

If you're interested in collaborating on this, myself and other Akonadi 
developers are available on the kde-pim mailing list and #akonadi on 
freenode.

All the best,

Steve.


> 
> Here's what I think doesn't work:
> 
> class NotReallyThreadSafeStandardItemModel :
> public QStandardItemModel
> {
> private:
> QReadWriteLock dataLock;
> public:
> bool insertRows(int row, int count, const QModelIndex& parent =
> QModelIndex()) {
> dataLock.lockForWrite();
> QStandardItemModel::insertRows(row, count, parent);
> dataLock.unlock();
> }
> QVariant data(const QModelIndex& index, int role = Qt::DisplayRole)
> const {
> dataLock.lockForRead();
> QVariant result = QStandardItemModel::data(index, role);
> dataLock.unlock();
> return result;
> }
> ...
> };
> 
> What I see as the problem is that the model/view updating strategy
> relies on signals and slots, and it's not expecting them to be queued.
> 
> For instance: the Gui View might see a model has 4 items, and set a
> variable ItemsDrawn to 4.  Then later it gets a notification saying
> "I'm adding one row", so it sets ItemsDrawn to 5 and tries to fetch
> the 5th element to draw.  But this could fail if that item has already
> been removed from the data by another thread.  This may have happened,
> but the "I'm deleting one row" notification is still pending in the
> Gui queue after the current "adding" notification we are processing...!
> 
> This would seem to suggest that it is not practical to have a model
> and view on separate threads.  As a thought-experiment, I've scribbled
> some rough code which tries for a "correct" 2-threaded
> StandardItemModel:
> 
> class TwoThreadedStandardItemModel : public QStandardItemModel {
> private:
> QReadWriteLock dataLock;
> QMutex insertMutex;
> QWaitCondition insertDone;
> bool insertSuccess;
> ...
> signals:
> bool insertRowsSignal(int row, int count, const QModelIndex & parent);
> protected slots:
> void insertRowsSlot(int row, int count, const QModelIndex & parent) {
> assert(currentThreadIsGui());
> assert(sender()->thread() == workerThread());
> dataLock.lockForWrite();
> bool success = QStandardItemModel::insertRows(row, count, parent);
> dataLock.unlock();
> 
> insertMutex.lock();
> insertSuccess = success;
> insertDone.wakeOne();
> insertMutex.unlock();
> }
> ...
> public:
> bool insertRow(int row, int count, const QModelIndex & parent =
> QModelIndex()) {
> bool success;
> if (currentThreadIsWorker()) {
> insertMutex.lock();
> emit insertRowsSignal(row, count, parent, false); // cross-thread,
> queued
> insertDone.wait(&insertMutex);
> success = insertSuccess;
> insertMutex.unlock();
> } else {
> dataLock.lockForWrite();
> success = QStandardItemModel::insertRows(row, count, parent);
> dataLock.unlock();
> }
> return success;
> }
> ...
> QVariant data(const QModelIndex & index, int role = Qt::DisplayRole)
> const {
> // reading does not need to use a signal
> dataLock.lockForRead();
> QVariant result = QStandardItemModel::data(index, role);
> dataLock.unlock();
> return result;
> }
> ...
> ThreadsafeStandardItemModel () {
> connect(this, SIGNAL(insertRowsSignal(int, int, const QModelIndex&,
> bool),
> this, SLOT(insertRowsSlot(int, int, const QModelIndex&, bool));
> ...
> }
> ...
> };
> 
> Despite the synchronization overhead, I imagine this approach could
> avoid crashes.  But if the View class you are using can do any
> autonomous modifications (like drag and drop rearrangement), then it
> could break the expectations of worker thread code, such as:
> 
> model->insertRow(0);
> // Gui could modify model here!
> model->setData(model->index(0, 0), subject);
> // Gui could modify model here!
> model->setData(model->index(0, 1), sender);
> // Gui could modify model here!
> model->setData(model->index(0, 2), date);
> 
> Any of those opportunities for modification could delete or move the
> row we are working with.  A better strategy is for the worker to do
> updates via larger atomic asynchronous operations queued to the GUI
> thread.  Each should have no reliance on the prior to have finished to
> perform the next, e.g.
> 
> public slots:
> void addMailMessage(const QString &subject, const QString &sender,
> const QDateTime &date)
> {
> assert(currentThreadIsGui()); // Modifications below will be atomic!
> model->insertRow(0);
> model->setData(model->index(0, 0), subject);
> model->setData(model->index(0, 1), sender);
> model->setData(model->index(0, 2), date);
> }
> 
> So... errr... AmiIRight? :)
> 
> Thanks!
> Brian
> 
> ---
> http://hostilefork.com