Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

Timer with Lambda function



  • I am sending JSON messages between processes and I want start a timer when the message is sent, if no acknowledge is received then the timer will timeout and the same message will be written again, at least that's the intention.

    I have a structure:

    typedef struct {
        QJsonObject objMsg;
        QTimer* pTimer;
    } tAckMsgTracking;
    
    typedef std::map<qulonglong, tAckMsgTracking*> mmpAck;
    

    The map is keyed by a message transaction ID which is uniquely generated when the message is sent. The problem I'm having is that the lambda slot does not get called:

    tAckMsgTracking* pAckMsgTrkr = new tAckMsgTracking;
    pAckMsgTrkr->objMsg = objJSON;
    pAckMsgTrkr->pTimer = new QTimer(this);
    QObject::connect(pAckMsgTrkr->pTimer, &QTimer::timeout, [this, pAckMsgTrkr]() {
    //Re-send message
        emit write(pAckMsgTrkr->objMsg);
    });
    //Insert a unique message ID into the message
    objJSON.insert(clsJSON::mscszMsgID, QString::number(++clsMsgSender::msulnglngMsgID));
    //Insert entry into the map
    clsMsgSender::msmpAcks.insert(std::make_pair(clsMsgSender::msulnglngMsgID, pAckMsgTrkr));
    //Start the timer that will resend message if ack. not received
    pAckMsgTrkr->pTimer->start(clsMsgSender::mscuint16AckTimeout);
    

    The value of 'mscuint16AckTimeout' is 5000 (milliseconds). I've also tried:

    pAckMsgTrkr->pTimer->start(std::chrono::milliseconds(clsMsgSender::mscuint16AckTimeout));
    

    I have a break point in the lambda slot, it doesn't get hit.


  • Lifetime Qt Champion

    @SPlatten said in Timer with Lambda function:

    pAckMsgTrkr->pTimer = new QTimer(this);

    Not related, but: why do you allocate timer on the heap?

    So, you don't get acknowledge and timeout slot is not called?



  • @jsulm, I create a timeout for each message that is sent as the queue doesn't wait one message to be sent successfully, before it sends another. So the timeout is for each message sent, the same queue can send to different hosts.


  • Lifetime Qt Champion

    @SPlatten I still don't see the need for heap allocation for pTimer...



  • @jsulm , what else would you suggest? I don't want to static allocate and I cannot use the stack.


  • Lifetime Qt Champion

    @SPlatten said in Timer with Lambda function:

    what else would you suggest?

    Make it member instead of a pointer? What's the point to have it as pointer allocate memory on the heap (which is slower) and then care to not to forget to delete it?



  • @SPlatten
    Since the code looks OK (other than heap issue), try a qDebug() instead of relying on breakpoint, and move the timeout down to 0 to debug.



  • @SPlatten said in Timer with Lambda function:

    I have a break point in the lambda slot, it doesn't get hit.

    There are only 2 things why slot is not called:

    • the thread in which the QTimer in living does not have a running event loop
    • the event loop is locked (by a QThread::sleep() or a forever loop)


  • I've re-written the code instead of a structure:

        class clsMsgTrkr : QTimer {
        private:
            static const quint16 mscuint16AckTimeout = 5000;
            static mmpAck msmpAcks;
    
            QJsonObject mobjMsg;
            clsMsgSender* mpMsgSndr;
    
        public:
            clsMsgTrkr(clsMsgSender* pMsgSndr, const QJsonObject& crobjJSON) {
                setInterval(mscuint16AckTimeout);
                mobjMsg = crobjJSON;
                mpMsgSndr = pMsgSndr;
        //Add tracker to list
                clsMsgTrkr::msmpAcks.insert(std::make_pair(clsMsgSender::ulnglngGetMsgID(), this));
    
                QObject::connect(this, &QTimer::timeout, [this]() {
                    qdbg() << "TIMEOUT!";
                    //Re-send message
                    emit mpMsgSndr->write(mobjMsg);
                });
                start();
            }
        };
    

    I can see in the debugger the constructor is getting called and processed the timer is started but I don't get anything in the slot.


  • Lifetime Qt Champion

    @SPlatten Don't know why you are now subclassing QTimer...
    You can add a destrcutor with debug output to see whether the instance is destroyed before timeout occurs.
    Also check what others wrote.



  • @SPlatten
    As @jsulm has just said. And as I suggested, put timeout down to 0 while you debug....



  • I added a call after call:

    start();
    qdbg() << this->isActive();
    

    qdbg is just a macro I use which is:

    #define qdbg()      qDebug().noquote().nospace()
    

    In the Application Output I see true so the timer is active, but doesn't timeout. Also, I just set the timer interval to 0, still no change, no timeout signal occurs.



  • @SPlatten said in Timer with Lambda function:

    In the Application Output I see true so the timer is active, but doesn't timeout. Also, I just set the timer interval to 0, still no change, no timeout signal occurs.

    Do you have a forever loop in your code or do you use QThread::sleep()?
    Are you sure QEventLoop of the used thread is working?



  • @KroMignon, the message transmission is in a thread and there very short sleep in the thread loop:

    void clsMsgSender::run() {    
        QJsonObject objJSON;
        while( blnAnythingToDo(objJSON) == true ) {
        //Sleep to allow a small cap between transmission
            QThread::usleep(100);
        //Look for a module name in the message
            QJsonObject::iterator itrFound = objJSON.find(clsJSON::mscszMsgType);
    
            if ( itrFound != objJSON.end() ) {
                const QJsonValueRef crobjMsgType = itrFound.value();
                QString strMsgType(crobjMsgType.toString());
    
                if ( strMsgType.compare(clsJSON::mscszAck) != 0 ) {
        //Insert a unique message ID into the message
                    objJSON.insert(clsJSON::mscszMsgID, QString::number(++clsMsgSender::msulnglngMsgID));
        //Create entry to monitor status of this message
                    new clsMsgTrkr(this, objJSON);
                }
            }
        //Writes message to socket
            emit write(objJSON);
        }
        emit queueEmpty();
    }
    

  • Lifetime Qt Champion

    @SPlatten You expect this to work?



  • @jsulm , ok, perhaps I'm to close to see the obvious, what have I done wrong? Its not complete yet, I haven't finished the code yet, but is there something that would explain why the timer isn't working?



  • @SPlatten I've told you many times to read basic Qt documentation.

    this cannot work!!!

    First, I suppose clsMsgSender is subclassing QThread. And you have create your own run() implementation. So there is no running QEventLoop. This means, all QObject which are running in this thread can NOT receive/emit signals.

    Second, you have a forever loop, so even if there where a running QEventLoop, it will not be called!



  • @KroMignon , I'm impatient, I know there is benefit from reading the documentation, I just don't want to stop what I'm doing to spend what would seem a long time to digest the documentation.



  • @SPlatten said in Timer with Lambda function:

    I'm impatient,

    Perhaps you are impatient, but in fact you are losing days doing nonsense code which not working.
    If that is the best way to work, I am pretty sure NO.

    Reading this document would take you 1 or 2 hours, how many hours have you spend to create those non working code?


  • Lifetime Qt Champion

    @SPlatten said in Timer with Lambda function:

    I just don't want to stop what I'm doing to spend what would seem a long time to digest the documentation

    So, instead you waste your time here?
    I really don't get the logic...



  • The logic is I was I was hoping this would be quicker...obviously wrong, no worries, I will read the documentation.



  • @SPlatten said in Timer with Lambda function:

    The logic is I was I was hoping this would be quicker...obviously wrong, no worries, I will read the documentation.

    The point is not to read all Qt documentation, but at least the basics to understand how Qt is working.
    There are very basic notion to know.
    This will help you to right build your software and to take advantage of Qt architecture.

    And, as Qt is a asynchronous framework, most of the time you don't have to create threads.
    One thing is very important, never look thread this also locks the QEventLoop and break major Qt feature ==> signals/slots handling.



  • @KroMignon , thanks for the advice, I've modified the thread loop to:

    void clsMsgSender::run() {    
        QJsonObject objJSON;
        while( blnAnythingToDo(objJSON) == true ) {
        //Sleep to allow a small cap between transmission
            exec();
            QThread::usleep(100);
        //Look for a module name in the message
            QJsonObject::iterator itrFound = objJSON.find(clsJSON::mscszMsgType);
    
            if ( itrFound != objJSON.end() ) {
                const QJsonValueRef crobjMsgType = itrFound.value();
                QString strMsgType(crobjMsgType.toString());
    
                if ( strMsgType.compare(clsJSON::mscszAck) != 0 ) {
        //Insert a unique message ID into the message
                    objJSON.insert(clsJSON::mscszMsgID, QString::number(++clsMsgSender::msulnglngMsgID));
        //Create entry to monitor status of this message
                    new clsMsgTrkr(this, objJSON);
                }
            }
        //Writes message to socket
            emit write(objJSON);
        }
        emit queueEmpty();
    }
    

    I've also added a class to replace the original structure:

        class clsMsgTrkr {
        Q_OBJECT
    
        private:
            static const quint16 mscuint16AckTimeout = 5000;
            static mmpAck msmpAcks;
    
            QJsonObject mobjMsg;
            clsMsgSender* mpMsgSndr;
            QTimer mtmrMonitor;
    
        public:
            clsMsgTrkr(clsMsgSender* pMsgSndr, QJsonObject& robjJSON);
            ~clsMsgTrkr();
    
        signals:
            void startTiming();
    
        public slots:
            void onStartTiming();
            void onTimeout();
        };
    

    The implementation:

    //Static initialisation
    mmpAck clsMsgTrkr::msmpAcks;
    /**
     * @brief clsMsgTrkr - Class constructor
     * @param pMsgSndr : Pointer to message sending class
     * @param robjJSON : Reference to JSON message
     */
    clsMsgTrkr::clsMsgTrkr(clsMsgSender* pMsgSndr, QJsonObject& robjJSON) {
        mobjMsg = robjJSON;
        mpMsgSndr = pMsgSndr;
    //Add tracker to list
        clsMsgTrkr::msmpAcks.insert(std::make_pair(clsMsgSender::ulnglngGetMsgID(), this));
    }
    /**
     * @brief clsMsgTrkr::~clsMsgTrkr - Class desctructor
     */
    clsMsgTrkr::~clsMsgTrkr() {
        if ( mtmrMonitor.isActive() ) {
            mtmrMonitor.stop();
        }
    }
    /**
     * @brief clsMsgTrker::onStartTiming
     */
    void clsMsgTrkr::onStartTiming() {
        QObject::connect(&mtmrMonitor, &QTimer::timeout, this, &clsMsgTrkr::onTimeout);
        mtmrMonitor.start(1);//mscuint16AckTimeout);
    }
    /**
     * @brief clsMsgTrkr::onTimeout
     */
    void clsMsgTrkr::onTimeout() {
        qdbg() << "TIMEOUT!";
        //Re-send message
        emit mpMsgSndr->write(mobjMsg);
    }
    

    Its still a work in progress, but I'm struggling with the compile errors:

    ../clsMsgSender.cpp:242:14: note: in instantiation of function template specialization 'QObject::connect<void (QTimer::*)(QTimer::QPrivateSignal), void (clsMsgTrkr::*)()>' requested here
        QObject::connect(&mtmrMonitor, &QTimer::timeout, this, &clsMsgTrkr::onTimeout);
    


  • @SPlatten
    If I'm not mistaken, you have not pasted the line before (or maybe after) the one you quote, which says what the actual error is.....



  • @SPlatten said in Timer with Lambda function:

    class clsMsgTrkr {
    Q_OBJECT

    Does not made sense, do you mean?

    class clsMsgTrkr : public QObject 
    {
         Q_OBJECT
    ...
    };
    


  • @KroMignon thank you, that was the problem I originally based the class on QTimer.



  • @SPlatten said in Timer with Lambda function:

    thank you, that was the problem I originally based the class on QTimer.

    Sorry, but that is only one of your problems.
    I know, each developer has his own programming style, but you are very confusing.

    You are mixing to many things and subclassing Qt classes which does not really required to be (like QTimer and QThread).

    And calling exec() in the run() slot will of course start the QEventLoop. But, all code after exec() will be executed on QEventLoop end!.

    There are so many errors, I don't know where to start.
    Sorry but please read this to understand how to use threads with Qt:



  • @KroMignon , ok thank you.



  • @KroMignon , I've just been reading the links that you posted.

    The first link is interesting in the it states: "Right because QThreads are in fact quite easy to use, as long as you ignore the incorrect official Qt documentation on QThread"

    I've been modifying some of my threads based on this information. The second link shows illustrations Usage 1-1 which is identical to the way I've implemented a class derived from QThread that does not call exec in the run function.

    I'm still reading and working through the documentation.



  • @SPlatten said in Timer with Lambda function:

    I'm still reading and working through the documentation.

    Great to see that you are starting reading documentation.

    What you should learn is that subclassing QThread (or QTimer) is in 99.9% case a nonsense, there are absolut no benefit.
    It is even more easy to create a worker class and move the instance to the dedicated thread.

    One reason is that QThread is a QObject which is holding a thread. So the QThread signals/slots are "living" in the thread in which the QThread instance has been created. But the QObject which are moved to the QThread instance are living in the holded thread.

    Another reason is that you could use the worker in current thread or in a dedicated thread without having to do any change.

    Try to think simple, things becomes complex quickly enough!



  • @KroMignon said in Timer with Lambda function:

    QThread

    Here is my new implementation of my debugging thread:

    if ( clsDebugService::mspThread == nullptr ) {
            clsDebugService::mspThread = new QThread;
            moveToThread(clsDebugService::mspThread);
            //connect(this, SIGNAL(error(QString)), this, SLOT(errorString(QString)));
            connect(clsDebugService::mspThread, SIGNAL(started()), this, SLOT(process()));
            //connect(this, SIGNAL(finished()), clsDebugService::mspThread, SLOT(quit()));
            connect(this, SIGNAL(finished()), this, SLOT(cleanup()));
            clsDebugService::mspThread->start();
    }
    

    mspThread is a static member of the class clsDebugService, clsDebugService is derived from QObject.

    The cleanup slot:

    void clsDebugService::cleanup() {
        if ( clsDebugService::mspThread != nullptr ) {
            if ( clsDebugService::mspThread->isFinished() != true ) {
                clsDebugService::mspThread->quit();
            }
            delete clsDebugService::mspThread;
            clsDebugService::mspThread = nullptr;
        }
    }
    

    The process slot:

    void clsDebugService::process() {
        QString strData, strPath(clsDebugService::strGetUserFolder());
        clsDebugService* pService = clsDebugService::pGetService();
    
        while ( pService != nullptr ) {
            {
        //Anything on the stack?
                QMutexLocker lock(&pService->mMutex);
    
                if ( pService->mStack.size() == 0 ) {
        //Nothing on the stack, terminate thread
                    break;
                }
                strData = pService->mStack.pop();
            }
            if ( strData.isEmpty() == true ) {
                continue;
            }
            QStringList slstLines = strData.split("\n");
            strData = "";
        //+2 to allow for type prefix and colon
            QString strPadding(" ");
            strPadding = strPadding.repeated(clsDebugService::mscintSeqNoLength + 2);
            uint16_t uint16Line = 1;
            foreach( QString strLine, slstLines ) {
                if ( uint16Line++ > 1 ) {
        //Insert padding at the front of additional lines
                    strLine = strPadding + strLine;
                }
                bool blnToFile;
                if ( pService->blnToFile(blnToFile) == true && blnToFile == true ) {
                    bool blnAutoFileName, blnOpen;
                    if ( pService->blnAutoFileName(blnAutoFileName) != true ) {
                        blnAutoFileName = false;
                    }
                    if ( blnAutoFileName == true ) {
                        QChar qcPadding('0'), qcSeperator(QDir::separator());
                        QDate dtNow(QDate::currentDate());
                        int intBase(10), intYear(dtNow.year()), intMonth(dtNow.month()), intDay(dtNow.day());
        //Generate file name using year, month, day
                        QString strExisting, strFileName = QString("%1%2%3%4").arg(mstrPrefix)
                                                                .arg(intYear, 4, intBase, qcPadding)
                                                                .arg(intMonth, 2, intBase, qcPadding)
                                                                .arg(intDay, 2, intBase, qcPadding);
                        pService->blnFileName(strExisting);
    
        //Is file open and is the size greater than the limit?
                        qint64 int64FileSize;
    
                        if ( pService->blnIsFileOpen(blnOpen) == true
                          && blnOpen == true
                          && pService->blnFileSize(int64FileSize) == true
                          && int64FileSize > pService->mcint64FileSizeLimit ) {
        //Increment part number
                            pService->muint8PartNumber++;
                        }
                        if ( blnOpen == true ) {
        //Get just the file name
                            int intFile = strExisting.lastIndexOf(qcSeperator);
                            strExisting = strExisting.mid(intFile + 1);
        //Remove the extension
                            strExisting = strExisting.mid(0, strExisting.indexOf("."));
                        }
                        if ( blnOpen != true || strExisting.compare(strFileName) != 0 ) {
                            if ( blnOpen == true ) {
                                pService->blnFileClose();
                                blnOpen = false;
                            }
                            pService->muint8PartNumber = 1;
                        }
        //Append part number
                        if ( blnOpen != true ) {
                            pService->blnSetFileName(strPath
                                                   + strFileName
                                                   + QString(".%1.log")
                                                   .arg(pService->muint8PartNumber
                                                       ,3, intBase, qcPadding));
                        }
                    }
        //Is file open?
                    if ( pService->blnIsFileOpen(blnOpen) == true && blnOpen != true ) {
                        pService->blnOpenFile(blnOpen);
                    }
                    if ( blnOpen == true ) {
                        QTextStream ts(&pService->mFile);
                        ts << strLine.toLatin1().data() << "\r\n";
                    }
                }
                bool blnToConsole;
                if ( pService->blnToConsole(blnToConsole) == true && blnToConsole == true ) {
        //Output to console
                    std::cout << strLine.toLatin1().data() << "\n";
                    std::cout << std::flush;
                }
            }
        }
        emit finished();
    }
    

    The problem I'm getting not is that as soon as the finished signal is emitted, the application halts on:

    std::abort();
    

    line 1914 of qlogging.cpp, is there anything I've done wrong here? I think the issue is because the thread hadn't finished before it was deleted...

    I am reading up on how to properly wait until it is stopped. Changed connections to:

    connect(clsDebugService::mspThread, &QThread::started
                                ,this, &clsDebugService::process);
    connect(this, &clsDebugService::finished
                                ,clsDebugService::mspThread, &QThread::quit);
    connect(clsDebugService::mspThread, &QThread::finished
                                ,this, &clsDebugService::cleanup);
    

    For some reason when I look at the stack tract I see:

     qFatal("QThread: Destroyed while thread is still running");
    


  • @SPlatten said in Timer with Lambda function:

    For some reason when I look at the stack tract I see:

    As I wrote before, QThread is a QObject which is managing a thread. You should never use delete with a QObject instance but use QObject::deleteLater() which will ensure instance will be deleted in his working thread.

    I would suggest you to change clsDebugService::cleanup():

    void clsDebugService::cleanup() {
        if(clsDebugService::mspThread)
            clsDebugService::mspThread->deleteLater();
        clsDebugService::mspThread = nullptr;
    }
    


  • @KroMignon after calling deleteLater is it still ok to set mspThread to nullptr ?

    I've changed the clean up to:

        if ( clsDebugService::mspThread != nullptr ) {
            clsDebugService::mspThread->deleteLater();
            clsDebugService::mspThread = nullptr;
        }
    

    Now I'm seeing in the Application Output:

    QObject::moveToThread: Current thread (0x101e67a70) is not the object's thread (0x0).
    Cannot move to target thread (0x101e91530)
    
    You might be loading two sets of Qt binaries into the same process. Check that all plugins are compiled against the right Qt binaries. Export DYLD_PRINT_LIBRARIES=1 and check that only one set of binaries are being loaded.
    

    When all the debug messages have been displayed, a new debug message will cause the thread to be created again, I think this is what causes the above message.

    Is it because the moveToThread function is called again for the same class again?

    Ok, I've fixed it, I don't allow the thread to be deleted at all, until the application is terminated, all I do now is restart the thread as long as the thread ptr exists.


  • Lifetime Qt Champion

    @KroMignon said in Timer with Lambda function:

    Great to see that you are starting reading documentation.

    ROFL



  • I usually just call exit() on the thread to stop the event loop, i.e. clsDebugService::mspThread->exit() in your case. Then there is some special quirk with QThreads: You can connect the thread's finished signal with the thread's deleteLater slot. Before the event loop actually stops it will first still handle the deleteLater slot for the thread:

    connect(clsDebugService::mspThread, &QThread::finished, clsDebugService::mspThread, &QThread::deleteLater);
    

    This is how you can clean up the thread once your done.


Log in to reply