How to cleanup timers properly?

[jpn]

I am using a thread with an external event loop. This thread explains why: Postin custom events to a subclass of QThread
Now I've got a nasty problem with cleaning up timers running in the event loop class.
I've got tons of messages saying:

"QObject::killTimer: timers cannot be stopped from another thread"

when closing the application.

Example code snippets below illustrate the problem:

An example event loop class:

class MyEventLoop : public QEventLoop
{
public:
    MyEventLoop(QObject* parent = 0) : QEventLoop(parent)
    {
        timers << startTimer(1000);
        timers << startTimer(2500);
    }
 
    ~MyEventLoop()
    {
        // this basically gets never called since the output is flooded by thousands of lines saying:
        // "QObject::killTimer: timers cannot be stopped from another thread"
        // and i am forced to kill the application process
        foreach (int id, timers)
            killTimer(id);
    }
 
protected:
    void timerEvent(QTimerEvent* event)
    {
        qDebug() << "ID: " << event->timerId();
    }
 
private:
    QList<int> timers;
};

class MyEventLoop : public QEventLoop
{
public:
    MyEventLoop(QObject* parent = 0) : QEventLoop(parent)
    {
        timers << startTimer(1000);
        timers << startTimer(2500);
    }

    ~MyEventLoop()
    {
        // this basically gets never called since the output is flooded by thousands of lines saying:
        // "QObject::killTimer: timers cannot be stopped from another thread"
        // and i am forced to kill the application process
        foreach (int id, timers)
            killTimer(id);
    }

protected:
    void timerEvent(QTimerEvent* event)
    {
        qDebug() << "ID: " << event->timerId();
    }

private:
    QList<int> timers;
};

To copy to clipboard, switch view to plain text mode 

An example thread class using an "external" event loop:

class MyThread : public QThread
{
public:
    MyThread(QObject* parent = 0) : QThread(parent) {}
 
    ~MyThread()
    {
        myeventloop->quit();
        // i have also tried "delete myeventloop;"
        myeventloop->deleteLater();
    }
 
protected:
    void run()
    {
        myeventloop = new MyEventLoop;
        myeventloop->exec();
    }
 
private:
    MyEventLoop* myeventloop;
};

class MyThread : public QThread
{
public:
    MyThread(QObject* parent = 0) : QThread(parent) {}

    ~MyThread()
    {
        myeventloop->quit();
        // i have also tried "delete myeventloop;"
        myeventloop->deleteLater();
    }

protected:
    void run()
    {
        myeventloop = new MyEventLoop;
        myeventloop->exec();
    }

private:
    MyEventLoop* myeventloop;
};

To copy to clipboard, switch view to plain text mode 

An example widget managing a thread like above:

class ThreadWidget : public QTextEdit
{
public:
    ThreadWidget(QWidget* parent = 0) : QTextEdit(parent)
    {
        mythread = new MyThread(this);
        mythread->start();
    }
 
    ~ThreadWidget()
    {
        // as far as i understand, quit() would have no effect
        // because the thread is running an external event loop
        mythread->terminate();
    }
 
private:
    MyThread* mythread;
};

class ThreadWidget : public QTextEdit
{
public:
    ThreadWidget(QWidget* parent = 0) : QTextEdit(parent)
    {
        mythread = new MyThread(this);
        mythread->start();
    }

    ~ThreadWidget()
    {
        // as far as i understand, quit() would have no effect
        // because the thread is running an external event loop
        mythread->terminate();
    }

private:
    MyThread* mythread;
};

To copy to clipboard, switch view to plain text mode 

[jacek]

Only QThread::run() is executed in a new thread. The rest belongs to a the thread where QThread instance was created (in your case, the GUI thread).

[jpn]

Ok, maybe it wasn't so good idea to use an external event loop after all. I decided to revise the code a bit so that the thread runs it's own event loop and there is just some object running timers instead of an event loop object. the object is instantiated before entering to the event loop and deleted right after returning from the event loop.

void MyThread::run()
{
	myobject = new MyObject;
	exec();
	delete myobject;
}

void MyThread::run()
{
	myobject = new MyObject;
	exec();
	delete myobject;
}

To copy to clipboard, switch view to plain text mode 

MyObject cleans up it's timers in it's destructor and the widget managing the thread calls quit() and wait() for the thread in it's destructor. Sending custom events, cleaning up timers and everything else works now flawlessly.

[high_flyer]

Your design is problematic since you are mixing view and functionality.
(Like your ThreadWidget which is a QTextEdit that has functionality, with threads even!)
This is in general is a bad practice since it makes your code inflexible and it breaks encapsulation (which is an OOP principle).
The GUI should be used for the view and let the work be done in functional modules.
This will also make the whole thread problems you have much simpler.

[jpn]

Your design is problematic since you are mixing view and functionality.
(Like your ThreadWidget which is a QTextEdit that has functionality, with threads even!)
This is in general is a bad practice since it makes your code inflexible and it breaks encapsulation (which is an OOP principle).
The GUI should be used for the view and let the work be done in functional modules.
This will also make the whole thread problems you have much simpler.

The architecture of the real application is very much more complicated than the supplied dummy example. There is some kind of network/worker thread doing it's work underneath. There must be some mechanism for the thread to know when to do and what. I don't find it so bad design if the application's main widget manages the thread by starting/quiting it in the beginning/end of the widget's life time, and by either emitting signals or posting custom events to it..

The actual problem was very simple. You cannot emit signals or post events ACROSS THREAD to a QThread descendant object. When using signals and slots, the slot will be executed in the main thread and when posting event's, the event gets posted in the main event loop. The correct way is to instantiate an object in QThread::run() and emit signals and/or post events directly to that object.

[high_flyer]

jpn:
My comment about the design in the code you posted is not directly connected to the problem you raised with the cross thread communication.
And since its a design issue, it really has nothing to do with the size of the code, but on it functionality.
Its a bad practice to mix view and functionality.
It is good practise to use encapsulation when programming C++ (OOP).
I offered it as a constructive comment, its ok if you don't agree with it.
And, it *could* be that with a correct design your inter thread communication problem would be solved or easer to handle.

While I don't mind if you agree with my above comment I do mind the following since its missleading others:

You cannot emit signals or post events ACROSS THREAD to a QThread descendant object.

This is not true.
From the docs:

It is important to keep in mind that the event loop may be delivering events to your QObject subclass while you are accessing the object from another thread.

And :

When using signals and slots, the slot will be executed in the main thread and when posting event's, the event gets posted in the main event loop.

No - when you send signal across threads its execution depends on what kind of a connection you made:
If its a direct connetion the slot will be executed in the thread in which the signal was emmited, that does not have to be the main thread.
If the connection you made is a qued connection then the slot will be executed in the thread in which the object that has the slot lives in, again, does not have to be the main thread.

I would advise you to read the chapter about threading in the Qt4 docs.

[jpn]

No - when you send signal across threads its execution depends on what kind of a connection you made:
If its a direct connetion the slot will be executed in the thread in which the signal was emmited, that does not have to be the main thread.

The issue is not about the connection type. I have used a queued connection. The "problem" is object's owner thread. As if you know, queued signals are delivered as events underhood. If the QThread object lives in the main thread as well, the stuff gets queued but later sent directly, not posted across thread. One could easily believe that queued signals and/or posted custom events would get delivered to the qthread object's own event loop. But that is not the case and I'm glad to know it now.

If the connection you made is a qued connection then the slot will be executed in the thread in which the object that has the slot lives in, again, does not have to be the main thread.

You are missing the point. I was talking about two objects both living in main thread. Regardless of fact that one of the objects is a QThread object and the connection is queued, the slot will be executed in the main thread since that's where the QThread object lives in.

Go ahead and test it by yourself:
http://qtnode.net/pastebin/800

[jpn]

jpn:
My comment about the design in the code you posted is not directly connected to the problem you raised with the cross thread communication.
And since its a design issue, it really has nothing to do with the size of the code, but on it functionality.
Its a bad practice to mix view and functionality.
It is good practise to use encapsulation when programming C++ (OOP).
I offered it as a constructive comment, its ok if you don't agree with it.
And, it *could* be that with a correct design your inter thread communication problem would be solved or easer to handle.

I simply couldn't resist to comment this furthermore. You are just judging pretty hard without knowing anything about the subject..

What if everything shown in the GUI is highly dependant on a network flow receiving tons of data items all the time. The network flow must be adjusted once in a while so that only required data items are ordered, otherwise the connection would get obstructed. Mostly anything done by the user causes also a change in the network flow. It's not that I'm having any freaking widget with a direct pointer to some thread object. There is a separate module handling all the network stuff. As I said, the example was just to illustrate the inter-thread communication problem I had.