need ideas for list/table implementation

[mzimmers]

*This should immediately show why the recommended way of using QThreads in the documentation, namely to sub-class it and implement your own run() function, is very wrong. *

Really and truly?

So, the docs are wrong? Still?

http://doc.qt.io/qt-5/qthread.html#details

Another way to make code run in a separate thread, is to subclass QThread and reimplement run(). For example:

I wish I knew who to believe...even better, there wouldn't be uncontested disagreement on this subject.

But I can go ahead and re-implement it as per the blog.

[VRonin]

The docs are not wrong, I think they are just misleading.
Subclassing run() is an option but that doesn't imply any other method/member of the QThread subclass or the instances on this subclass live on a separate thread as you, me and a lot of other people intuitively thought the first time we approached QThread

[mzimmers]

OK, here's my changed main():

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    Widget widget;
    QThread* thread = new QThread;
    Worker* worker = new Worker();
    worker->moveToThread(thread);
 ...
    QObject::connect(thread, &QThread::started, worker, &Worker::process);
    QObject::connect(&widget, &Widget::quitButtonPushed, worker, &Worker::doQuit);
    QObject::connect(worker, &Worker::finished, thread, &QThread::quit);
    QObject::connect(worker, &Worker::finished, worker, &Worker::deleteLater);
    QObject::connect(thread, &QThread::finished, thread, &QThread::deleteLater);

    widget.show();
    thread->start();
    rc = a.exec();
    return rc;
}

First order of business: how does this look?
Second order of business: why does my quit button no longer work?

void Widget::on_quitButton_clicked()
{
    emit quitButtonPushed(0);
}
...
void Worker::doQuit()
{
    emit finished();
}

Thanks. Once I get this thread stuff straightened out, I think I'll close this topic and start a new one on the subject of model/view.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

This should immediately show why the recommended way of using QThreads in the documentation, namely to sub-class it and implement your own run() function, is very wrong.

Really and truly?
So, the docs are wrong? Still?
I wish I knew who to believe...even better, there wouldn't be uncontested disagreement on this subject.

Of course it isn't wrong to subclass and reimplement run(), she's overplaying it. However, both approaches have their specifics that should be taken into account, which @VRonin hinted. Firstly and most importantly is that QThread is not a thread, it's a class that manages a running thread. This also means that the object that manages the thread is not in the running thread (in the sense of thread affinity), and the distinction is important.

This implies for each of the approaches:

1. When you're subclassing and overriding run() that is your thread, literally the QThread::run() override is the whole thread. You can't store things in the thread object, because the thread object is "living" in another thread and all its slots will be called from a thread different than the one that is QThread::run(). Additionally, since there's no event loop in that case you can't have any events posted in the thread (slots can't be triggered from signal emissions coming from other threads with the exception of direct connections, signal emissions are fine to do; no timers can be run in that thread and so on).

   This approach is usually employed for the rarer case when you need to run an imperative code that doesn't care much about events and such, and just does something that it communicates to the outside world. E.g. some number crunching thread that does its business and spits out results. The thread ends with you returning from QThread::run(); also meaning you should handle the termination condition (semi-)manually (e.g. QThread::requestInterruption and the corresponding getter).

2. When you're using a worker object, you have a running event loop that can process queued events and have timers running. However, you should make sure that you're not blocking the event loop for too long ... yeah, every time I see Sleep a little piece of me dies in horrible pain ...

   This approach is the usual choice, and allows you nice separation of what data is accessed in what thread - i.e. the data contained in the instance of the QObject of some thread is touched only from that same thread. Connecting with signal-slots between the threads allows for Qt to give you data access serialization out of the box, without you needing to use mutexes, semaphores and other such nastiness. In contrast, though, concurrency is hurt a bit compared to 1, but that's irrelevant for 99.9% of possible applications.

First order of business: how does this look?

At a glance looks okay.

Second order of business: why does my quit button no longer work?

Do you run an endless loop in Worker::process? If so, you're blocking the event loop. It ain't gonna work like that.

[mzimmers]

Thanks for the detailed explanation, kshugenov.

So, regarding cross-thread communication, if the worker thread's infinite loop is blocking the event loop, does that imply that the worker won't respond to any signals at all? And if so, what's the recommended method for killing another thread? I tried this:

    QObject::connect(&widget, &Widget::quitButtonPushed, thread, &QThread::quit);

And even that doesn't work.

Thanks...

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

So, regarding cross-thread communication, if the worker thread's infinite loop is blocking the event loop, does that imply that the worker won't respond to any signals at all?

Yes, exactly. Blocking the event loop means no event processing, means no queued signal-slot calls, means slots won't be called until the event processing resumes. Any signals that are connected to slots in the worker through Qt::AutoConnection, Qt::QueuedConnection, or Qt::BlockingQueuedConnection are not called. This doesn't apply to Qt::DirectConnection, because direct connections don't require an event loop and event processing - the same as a direct calls.

And if so, what's the recommended method for killing another thread?

This is the correct way of stopping the thread when you're using the worker object, you just have to rework a bit your code so it doesn't block the event loop. For example substituting the while (true) for a timer with some timeout (or zero timeout). :)

[mzimmers]

I do have a sleep in my loop:

void Worker::process()
{
    int len;
    while (true)
    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
        Sleep(10);
    }
    //emit reachedEndOfThread();
}

(I renamed my worker routine from run() to process() to agree with the example I'm trying to follow.)

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

sleep() makes me cringe every time. Yes, I am saying that sleep() puts the thread to sleep and it does nothing, no event processing, literally nothing. You can't even interrupt that! In your case, looking at the code, the very simple solution would be to have something like this:

class Worker : public QObject
{
public:
    Worker();

public slots:
    void start();
    void process();

private:
    QTimer pollTimer;
};

Worker::Worker()
    : pollTimer(this) //< Important so the timer is moved to the new thread along with the worker
{
    pollTimer.setInterval(10000); // 10 seconds or so
}

void Worker::start()
{
    pollTimer.start();
}

void Worker::process()
{
    int len = sm.recv(buffIn, sizeof(buffIn));
    // And more of whatever it is you do INSIDE the `while (true)`
}

Also you seem to be using the native sockets, why?

PS.
Connect the QThread::started to Worker::start and it should be working out of the box.

[mzimmers]

Thanks for the example. Don't I need to connect the timer to my connect() routine somewhere, though?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Don't I need to connect the timer to my connect() routine somewhere, though?

Yeah, I missed it, sorry about that. You should have one such line:

QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);

in Worker's constructor.

[mzimmers]

OK, I think I'm almost there. Now, what's the recommended behavior of the slot that receives the quit button signal? exit()? quit()? Something else?

[kshegunov]

The quit button signal you should connect to QThread::quit, just as you had shown above. That's the way to do it.

[mzimmers]

Hmm...it's not working. I read this in the QThread page:

This function [quit] does nothing if the thread does not have an event loop.

Do I need to explicitly declare/start an event loop?

[VRonin]

If you don't subclass QThread the native QThread::run starts an event loop. Can you show us your current Worker::process implementation?

[mzimmers]

Sure:

void Worker::process()
{
    int len;
//    running.ref(); // set to value of 1
//    while (true)
//    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
//        Sleep(10);
//    }
    //emit reachedEndOfThread();
}

[VRonin]

emit newMessage(&msg) is a race condition, msg might (and almost surely does) go out of scope before the slots connected to the signal can use the argument

[mzimmers]

Noted, but I don't think that's causing my quit button to be ignored, is it?

I just realized my process() routine is only called once. Is there something wrong with my pollTimer?

Worker::Worker(QObject *parent) : QObject (parent), devices(parent), pollTimer(this)
{
    QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);
    pollTimer.setInterval(100);
}
void Worker::start()
{
    pollTimer.start();
}

[VRonin]

This is weird... can you post the entire worker.h and worker.cpp?

Hmm...it's not working.

How are you testing if it's working or not?

P.S.
No need to give parents to stack-allocated QObjects like devices or pollTimer
edit: wrong in this case, without a parent the objects will belong to the thread calling Worker constructor and not be moved with it

[mzimmers]

worker.h:

#ifndef WORKER_H
#define WORKER_H

#include "winsock2.h"
#include "ws2tcpip.h"

#include <QAtomicInt>
#include <QObject>
//#include <QThread>
#include <QTimer>

#include "constants.h"
#include "devices.h"
#include "message.h"
#include "socket.h"

namespace Ui
{
class Widget;
}

class Worker : public QObject //QThread
{
    Q_OBJECT

private:
    SOCKET sock;
    SocketMC sm;
    int myErrno;
    Message msg;
    char msgOut[1024];
    char buffIn[1024];
    Devices devices;
    QTimer pollTimer;

    void sendHeartbeatAck();
    void updateDeviceTable(Message msg);
public:
    explicit Worker(QObject *parent = nullptr);
    ~Worker();
    void readSocket();
    void doQuit();
signals:
    void newMessage(Message *msg);
    void finished();
//    void reachedEndOfThread(int rc = 0);

public slots:
    void start();
    void process();
    void sendLedPatternChange(LedPattern newState);
    void sendLedBrightnessChange(int value);
    void sendScanRequest();
};

#endif // WORKER_H

worker.cpp:

#include <iostream>
#include <string>
#include <stdio.h>

#include <QDateTime>

#include "constants.h"
#include "message.h"
#include "worker.h"

using namespace std;

Worker::Worker(QObject *parent) : QObject (parent), devices(parent), pollTimer(this)
{
    DeviceDetails dev;

//    dev.macAddr = "macaddr2";
//    dev.devName = "devname22";
//    dev.latestHB = "hbtime222";
//    devices.update(dev);

//    dev.macAddr = "macaddr55";
//    dev.devName = "devname55";
//    dev.latestHB = "hbtime55";
//    devices.update(dev);

    QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);
    pollTimer.setInterval(100);
}
Worker::~Worker()
{
}
void Worker::doQuit()
{
//    running.deref(); // set to value of 0.
    emit finished();
}

void Worker::start()
{
    pollTimer.start();
}

void Worker::sendLedPatternChange(LedPattern newState)
{
    Message msg;

    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_LED_SET_PATTERN]);
    msg.add(msgTags[TAGENUM_NEWPATTERN], ledPatternText[newState]);

    sm.send(msg.encodeXml());
}
void Worker::sendLedBrightnessChange(int value)
{
    Message msg;
    string s;

    s = std::to_string(value);
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_LED_SET_BRIGHTNESS]);
    msg.add(msgTags[TAGENUM_NEWBRIGHTNESS], s);

    sm.send(msg.encodeXml());
}

void Worker::sendHeartbeatAck()
{
    Message msg;
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_HEARTBEAT_ACK]);

    sm.send(msg.encodeXml());
}

void Worker::updateDeviceTable(Message msg)
{
    DeviceDetails dev;
    dev.macAddr = QString::fromStdString(msg.getValue(msgTags[TAGENUM_MACADDRESS]));
    dev.devName = QString::fromStdString(msg.getValue(msgTags[TAGENUM_DEVICENAME]));
    dev.latestHB = QDateTime::currentDateTimeUtc().toString(QString::fromStdString(timestampFormat));

    devices.update(dev);
}
void Worker::sendScanRequest(void)
{
    Message msg;
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_SCAN_REQUEST]);

    sm.send(msg.encodeXml());
}
void Worker::process()
{
    cout << "worker::process() started." << endl;
    int len;
//    running.ref(); // set to value of 1
//    while (true)
//    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            msg.decodeXml(buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
//        Sleep(10);
//    }
    //emit reachedEndOfThread();
}

The test for whether the exit works is if the window disappears, and the debugger exits. It doesn't. The test for how often process() is called is the cout at the top of the routine.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Noted, but I don't think that's causing my quit button to be ignored, is it?

Probably not, although it would eventually cause a segafault somewhere.

I just realized my process() routine is only called once. Is there something wrong with my pollTimer?

Not that I can see. Are you sure this:

len = sm.recv(buffIn, sizeof(buffIn));

isn't blocking? (also the reason for my question why aren't you using Qt's sockets)