Restart a QThread

[P51D]

Hi

I've some troubles with threading. In my application I've separated the GUI and a streaming/processing part into 2 threads. The streaming thread is created like the example in the QThread Class documentation. For a "single shot" that works great (starting and stopping the streaming thread). But if I re-click the streaming button the application crashes. There is no difference if I call the destructor for my background-worker class and recreate a complete new instance, or if I only re-call the thread start function.

Where is my problem?

Here are the specific parts:


MainWindow::MainWindow(QWidget *parent) :
QMainWindow(parent),
ui(new Ui::MainWindow)
{
backgroundWorker = new workerThread;
//Setup callback for cleanup when it finishes
connect(backgroundWorker, SIGNAL(finished()), backgroundWorker, SLOT(deleteLater()));
connect(backgroundWorker, SIGNAL(FrameReady()), this, SLOT(update_detection_lb()));

connect(ui->StartStopStreaming_pb, SIGNAL(clicked()), this, SLOT(Streaming()));

// Set gui labels
ui->Gain_lb->setText(QString("%1 dB").arg(backgroundWorker->gain));
ui->fc_lb->setText(QString("%1 MHz").arg(backgroundWorker->freq / 1e6));
ui->adr_lb->setText(QString::fromStdString(backgroundWorker->addr));
}

// Streaming button
void MainWindow::Streaming()
{
if(ui->StartStopStreaming_pb->text() == "Start streaming")
{
ui->StartStopStreaming_pb->setText("Stop streaming");

std::cout << "Start streaming in background thread" << std::endl;

if(backgroundWorker)
{
backgroundWorker->m_abort = false;
backgroundWorker->start(QThread::HighestPriority);
}
}
else{
ui->StartStopStreaming_pb->setText("Start streaming");

std::cout << "Stop streaming and terminate background thread" << std::endl;

backgroundWorker->m_abort = true;
if(!backgroundWorker->wait(5000))
{
std::cout << "Thread deadlock detected, bad things may happen!" << std::endl;

backgroundWorker->terminate();
backgroundWorker->wait();
}
}
}

void workerThread::run()
{
std::cout << "Thread running" << std::endl;

while(m_abort == false)
{
// Do some streaming stuff
}

std::cout << "Thread terminated" << std::endl;
emit finished();
}


Best regards,
P51D

[anda_skoa]

A couple of things:

1) access to workerThread::m_abort is not properly synchronized between the two threads.
2) backgroundWorker gets deleted when it finishes but there doesn't seem to be any code to reset the variable, yet there is an if (backgroundWorker)
3) QThread::terminate() is a bad thing

Cheers,
_

[P51D]

1) access to workerThread::m_abort is not properly synchronized between the two threads.

I know the "proper" way would be using mutex or signal/slots. But in this case I'm only reading the variable from one thread and writing it from the other...



2) backgroundWorker gets deleted when it finishes but there doesn't seem to be any code to reset the variable, yet there is an if (backgroundWorker)

I don't know what you mean. Can you explain it in other words?



3) QThread::terminate() is a bad thing

I know, but this is only an "emergency" solution, if the thread blocks about 5 sec.

Best regards

[anda_skoa]

I know the "proper" way would be using mutex or signal/slots. But in this case I'm only reading the variable from one thread and writing it from the other...

Read/write from different threads is what needs to be synchronized.
Only reading from two threads is OK without protection.

But for this case you could also have a look at QThread::requestInterruption() which already implements that.



I don't know what you mean. Can you explain it in other words?

You have connected the thread's finished() signal to its deleteLater() slot, so the thread is deleted when the thread ends.
But the pointer variable is not reset anywhere I can see.



I know, but this is only an "emergency" solution, if the thread blocks about 5 sec.

Why do you even need to wait for the thread to end?
It gets deleted once it finishes, no matter if it does that after 1 millisecond or after 10 seconds.

Cheers,
_

[P51D]

Some updates:

I managed to synchronize the stop call with requenstInterruption. But the other thing with restarting the thread wont work. It doesn't matter if I delete the object like this and try to create a new instance:



delete backgroundWorker;
workerThread* backgroundWorker = new workerThread;


I'm a bit confused.

[anda_skoa]

If that is your actual code then you have just created a local variable that shadows the member variable with the same name.

Cheers,
_

[jefftee]

Once you fix the shadow variable issue that @anda_skoa pointed out, you'll need to redo your connects after you've created the new workerThread, it is a new instance, etc.

[P51D]

Sorry, my fault. Off course it is not a local member...

For better understanding I have here the code (sry, this should be the first thing when asking for help).

Main window


class MainWindow : public QMainWindow
{
Q_OBJECT

public:
explicit MainWindow(QWidget *parent = 0);
~MainWindow();

public slots:
void update_detection_lb();
void Streaming();

private:
Ui::MainWindow *ui;

// UHD backround worker thread
workerThread *backgroundWorker;
};




MainWindow::MainWindow(QWidget *parent) :
QMainWindow(parent),
ui(new Ui::MainWindow)
{
backgroundWorker = NULL;
backgroundWorker = new workerThread;
//Setup callback for cleanup when it finishes
connect(backgroundWorker, SIGNAL(finished()), backgroundWorker, SLOT(deleteLater()));

// Setup signal slot connections
connect(ui->StartStopStreaming_pb, SIGNAL(clicked()), this, SLOT(Streaming()));
connect(backgroundWorker, SIGNAL(FrameReady()), this, SLOT(update_detection_lb()));

}


void MainWindow::Streaming()
{
if(ui->StartStopStreaming_pb->text() == "Start streaming")
{
ui->StartStopStreaming_pb->setText("Stop streaming");

if(backgroundWorker == NULL)
{
backgroundWorker = new workerThread;
connect(backgroundWorker, SIGNAL(finished()), backgroundWorker, SLOT(deleteLater()));
connect(backgroundWorker, SIGNAL(FrameReady()), this, SLOT(update_detection_lb()));
}
backgroundWorker->start(QThread::HighestPriority);

std::cout << "Start streaming in background thread" << std::endl;
}
else{
ui->StartStopStreaming_pb->setText("Start streaming");

backgroundWorker->requestInterruption();
if(!backgroundWorker->wait(5000))
{
std::cout << boost::format("Thread deadlock detected, bad things may happen!") << std::endl;
backgroundWorker->quit();
backgroundWorker->wait();
}
std::cout << "Stop streaming and terminate background thread" << std::endl;
delete backgroundWorker;
}
}


Worker thread


class workerThread : public QThread
{
Q_OBJECT

public:
workerThread();
~workerThread();

protected:
void run();

signals:
void finished();
void FrameReady();

private:

// UHD member
uhd::usrp::multi_usrp::sptr usrp;
uhd::rx_streamer::sptr rx_stream;
};


workerThread::workerThread()
{
std::cout << "Thread created" << std::endl;
}

workerThread::~workerThread()
{

}


void workerThread::run()
{
std::cout << "Thread running" << std::endl;

while(!this->isInterruptionRequested())
{
// Do streaming stuff
emit FrameReady();
}

std::cout << "Thread terminated" << std::endl;
emit finished();
}


The Problem is still the same. During the first start/stop all works fine. But when I try to restart the thread, I don't come into the if(backgroundWorker == NULL) and the program hangs at the backgroundWorker->start line.

[jefftee]

Perhaps set background worker to null after you delete it in line 66?

[anda_skoa]

The Problem is still the same. During the first start/stop all works fine. But when I try to restart the thread, I don't come into the if(backgroundWorker == NULL) and the program hangs at the backgroundWorker->start line.

You are artifically creating a different situation for startup vs. runtime by creating and connecting a thread in the constructor.

As far as I can tell the only thing the slot would need to do is to disconnect() from the old thread if it exists and create and start the new thread.
Since the thread is deleted via deleteLater(), you will need a QPointer for "backgroundWorker" so it can track if the object still exists.


void MainWindow::Streaming()
{
if(ui->StartStopStreaming_pb->text() == "Start streaming")
{
ui->StartStopStreaming_pb->setText("Stop streaming");

backgroundWorker = new workerThread;
connect(backgroundWorker, SIGNAL(finished()), backgroundWorker, SLOT(deleteLater()));
connect(backgroundWorker, SIGNAL(FrameReady()), this, SLOT(update_detection_lb()));
backgroundWorker->start(QThread::HighestPriority);

std::cout << "Start streaming in background thread" << std::endl;
}
else{
ui->StartStopStreaming_pb->setText("Start streaming");

if(backgroundWorker != 0)
{
disconnect(backgroundWorker, SIGNAL(FrameReady()), this, SLOT(update_detection_lb()));
backgroundWorker->requestInterruption();
}
}
}



Alternatively you could look into keeping the same thread object and just making it "pause/resume".

Cheer,
_