C++ QThread and connect causing crashes

Question

My QThread counter crashes giving odd results for what the number should be as the Thread counts properly but in the SetLabel function I get a different number to whats in the QThread ands it then crashes after 3 seconds and the label doesnt seem to update.

QThread* CountThread = new QThread;
Counter* Count = new Counter();
Count->moveToThread(CountThread);
connect(CountThread, SIGNAL(started()), Count, SLOT(Process()));
connect(Count, &Counter::SecondsUpdate, this, [=]{ SetLabel(Count->Seconds) ;});
connect(Count, SIGNAL(finished()), CountThread, SLOT(quit()));
CountThread->start();


void Counter::Process()
{
int secs = 0;
while (secs < 1000)
{
    qDebug() << "hello" << secs;
    secs += 1;
    Sleep(1000);
    emit SecondsUpdate();
}
emit finished();
}


void BaseWindow::SetLabel(int Seconds)
{
qDebug() << Seconds;
this->Test->setText(QString("Seconds: " + QString::number(Seconds)));
}

class Counter : public QObject
{
Q_OBJECT
public slots:
    void Process();

signals:
    void finished();
    void SecondsUpdate();

public:
    int getValue() { return Seconds;}
    int Seconds;

};

EDIT: The issue seems to lie in the changing of the label as I commented this->Text->setText out and it didnt crash

@eyllanesc class Counter : public QObject { Q_OBJECT public slots: void Process(); signals: void finished(); void SecondsUpdate(); public: int getValue() { return Seconds;} int Seconds; }; — mrdeadguy34, Jul 01 '18 at 09:42
Also note that `Counter::Process` never updates `Counter::Seconds`. — G.M., Jul 01 '18 at 09:45
@G.M.oooh that would make sense. It still crashes the program however. — mrdeadguy34, Jul 01 '18 at 09:47
You're also (in the lambda) accessing `Count->Seconds` from a thread *other* than that associated with `Count`. — G.M., Jul 01 '18 at 09:55
@mrdeadguy34 I'm in Linux so change Sleep() to QThread::Sleep(), although your implementation is not the most elegant I have implemented it and it does not generate any errors, you could take the time and provide a decent [mcve], besides using a debugger. — eyllanesc, Jul 01 '18 at 09:56
@mrdeadguy34 just a nit-pick (as I am sure this is simply a test program), but for the record I am obligated to mention standard coding convention names, see: https://stackoverflow.com/a/3706453/4628115 and https://stackoverflow.com/a/3706598/4628115 — CybeX, Jul 01 '18 at 11:10
Get rid of the SIGNAL and SLOT macro's they will only bite you runtime. — Surt, Jul 01 '18 at 11:43

G.M. · Accepted Answer · 2018-07-01T11:02:26.730

The code shown has two basic problems.

Counter::Seconds is never initialized/updated.
Having moved Count to a new QThread you continue to access it from the main thread.

You could solve both of these by getting rid of the Seconds member and just passing the local counter secs as a parameter to the Counter::SecondsUpdate signal...

class Counter: public QObject
{
  Q_OBJECT;
public slots:
  void Process();
signals:
  void finished();
  void SecondsUpdate(int seconds);
public:
};

void Counter::Process ()
{
  int secs = 0;
  while (secs < 1000)
  {
    qDebug() << "hello" << secs;
    secs += 1;

    /*
     * Sleep(1000) --> QThread::sleep(1)
     */
    QThread::sleep(1);

    /*
     * Pass secs as a parameter to the SecondsUpdate signal.
     */
    emit SecondsUpdate(secs);
  }
  emit finished();
}

Then, change the relevant connect call from...

connect(Count, &Counter::SecondsUpdate, this, [=]{ SetLabel(Count->Seconds) ;});

to...

connect(Count, &Counter::SecondsUpdate, this, &BaseWindow::SetLabel);

change `connect(Count, &Counter::SecondsUpdate, this, [=](int seconds) { SetLabel(seconds); });` to `connect(Count, &Counter::SecondsUpdate, this, &BaseWindow::SetLabel)`, is more elegant — eyllanesc, Jul 01 '18 at 10:52

score 0 · Answer 2 · answered Jul 01 '18 at 09:58

0

Fixed by passing through the label as a pointer and edited it in the function.

answered Jul 01 '18 at 09:58

mrdeadguy34

85
1
8

C++ QThread and connect causing crashes

2 Answers2