Qt bug? Signal emitted once, slot called multiple times

[MattPhillips]

Hi,

I'm getting this really strange situation, on multiple (unix-based) platforms, whereby a signal that I emit once results in multiple calls to the same slot. The slot does time-critical inter-process communication via socket connections, so this is pretty debilitating. Here's the signal:


class MyClass
{
Q_OBJECT
...
signals:
void WriteToProc(const QByteArray& qb, int* num_bytes = NULL);
};

MyClass::SendData(...)
{
QByteArray qb;
... //Fill up qb;
qDebug() << "Emitting signal";
emit WriteToProc(qb);
}

Here's the slot (has the same name):


class MyWorkerThread : public QThread
{
Q_OBJECT
...
public slots:
void WriteToProc(const QByteArray& qb, int* num_bytes);
private:
QLocalSocket* socket_out;
};

void MyWorkerThread::WriteToProc(const QByteArray& qb, int* num_bytes)
{
if (num_bytes==NULL) socket_out->write(qb);
else *num_bytes = socket_out->write(qb);
qDebug() << "MyWorkerThread::WriteToProc";
}

There's some stuff in there that's not directly relevant to the point that I include for completeness. SendData() and WriteToProc are in different threads, and the connection type is default (Qt::QueuedConnection). Anyway, the output is e.g.


Emitting signal
MyWorkerThread::WriteToProc
MyWorkerThread::WriteToProc
MyWorkerThread::WriteToProc

Moreover, looking at the output at the other end, I can see that the QByteArray is getting sent 3 (in this case; it can vary) times. I've verified that only SendData() is calling WriteToProc at that point. It's so weird--what could be going on?

Best,
Matt

[Lykurg]

The interesting part is, how you establish the connection. Most probably you establish the connection not only once. Try to use Qt::UniqueConnection on (all) connect statements.

[high_flyer]

is WriteToProc () connected to other signals as well?
Do a full search in your project for the connect() calls that involve this slot.
If there is this slot is connected to another signals, figure out why this signal fires, maybe by placing a debug output as you did here.

[wysota]

You do realize that the WriteToProc() slot will be called from the context of the main thread and not the context of the worker thread, right?

[MattPhillips]

Hi,

Thanks all for getting back to me. The program--and the relevant sections in particular, isn't *that* big, maybe 4K lines, and I'm the sole author. The connect statement connecting signal and slot is made only once, and WriteToProc (signal) and WriteToProc (slot) are connected exclusively to each other. As for Qt::UniqueConnection, I can try that anyway, can it just be |'ed with the others? I guess I'll experiment and see. wysota--


You do realize that the WriteToProc() slot will be called from the context of the main thread and not the context of the worker thread, right?

No I did not! Here's my connect statement:


MyWorkerThread::run()
{
....
connect(&expt, SIGNAL(WriteToProc(const QByteArray&,int*)), this, SLOT(WriteToProc(const QByteArray&,int*)));
exec();
}

I also have 'moveToThread(this)' in MyWorkerThread's constructor. I thought the above produced a Qt::QueuedConnection and these executed the slot in the context of the worker thread. Is that wrong? Or did you mean, the signal is emitted in the context of the main thread? If the latter, yes, I know.

Matt

[wysota]

If you have moveToThread() in the constructor then the slot will be called in the right context. But you shouldn't move the QThread object it its own thread. What's the point of having this thread anyway? I don't know how much code you cut out from the snippet you pasted here but it seems the thread is sitting there doing nothing.

[MattPhillips]

Hi wysota,

Where should I move the QThread object? Anyway, the worker thread is for sending information to the secondary process, which runs graphics and needs to be updated every video frame. There has to be a separate thread for this so that sending this info won't get delayed by e.g. time-consuming gui events of the primary process. It's just a standard i/o worker thread.

Matt

[wysota]

Where should I move the QThread object?
You shouldn't move it anywhere.


Anyway, the worker thread is for sending information to the secondary process, which runs graphics and needs to be updated every video frame. There has to be a separate thread for this so that sending this info won't get delayed by e.g. time-consuming gui events of the primary process.
You don't need a thread for that.

It's just a standard i/o worker thread.
"Standard" according to what specifications? In Qt you don't need threads for i/o because all operations are asynchronous.

[MattPhillips]

Hi wysota,

Communication between the main and secondary processes is frequent (every video frame, at least) and has to be reliable. If a time-consuming gui operation is happening in the main process, and I didn't have a separate i/o thread, then even though the messages are getting received they are not getting processed, which would lead to a failure of my application. So I don't see how I can do this without a separate thread. As for 'standard', I just meant that this is a standard function of i/o threads; take e.g. the QThread main documentation page, where a single example of a QThread subclass is given and that example is for managing a QTcpSocket.

Anyway, I have 'solved' the problem by doing a direct function call:

thd->WriteToProc(qb);

instead of

emit WriteToProc(qb);

where thd is the instance of MyWorkerThread. Now the slot is just getting called once. I'd still like to know what's going on in the first place though.

Matt

[tbscope]

As for 'standard', I just meant that this is a standard function of i/o threads;
Understand that the QThread class by itself is NOT a different thread.


take e.g. the QThread main documentation page, where a single example of a QThread subclass is given and that example is for managing a QTcpSocket.
This is a very unfortunate example. In my opinion, it should be removed from the documentation and replaced by something more useful.


Anyway, I have 'solved' the problem by doing a direct function call:

thd->WriteToProc(qb);

instead of

emit WriteToProc(qb);

where thd is the instance of MyWorkerThread. Now the slot is just getting called once. I'd still like to know what's going on in the first place though.

Do you know what this code does? Can you explain it?
Please do not take this in the wrong way. I'm trying to help you understand why what you did is completely wrong. Even from the beginning (your first post).

What you should do is this:
1. Create your gui application like you normally do
2. Create a QObject subclass (the worker class) that will act as your worker thread. Yes, do not subclass QThread!
3. Create a QThread object and an object based on your worker class. You can make both objects members of your main window and instantiate them in the constructor.
4. Connect the signals and slots.
5. Move the object to the worker thread. Note again that the QThread object is not the new thread, it does manage a new thread though.
6. Whenever you're ready, start the thread.

Since you moved the worker object completely to a new thread, the signals and slots are called in the worker thread.

[wysota]

5. Move the object to the worker thread. Note again that the QThread object is not the new thread, it does manage a new thread though.

To help visualize it, consider that QThread object has a similar relationship to the thread it controls as QProcess object has to the process it controls.


If a time-consuming gui operation is happening in the main process, and I didn't have a separate i/o thread, then even though the messages are getting received they are not getting processed, which would lead to a failure of my application. So I don't see how I can do this without a separate thread.
A separate thread will not guarantee that.

[MattPhillips]

Hi,

Thanks a lot to both of you, especially tbscope for your instructions. I will definitely try to reorganize my code along those lines, though I probably can't get to it right away. It sounds more elegant than what I'm doing now. But as for what 'thd->WriteToProc(qb)' does, do I understand it? What level of understanding do you have in mind? thd is a pointer to a MyWorkerThread instance, '->' means 'access the member of', qb is a QByteArray, and WriteToProc is a member function of MyWorkerThread that sends qb to the auxiliary process using a QLocalSocket. 'emit WriteToProc(qb)' is essentially the same thing if the connection is a Qt::DirectConnection (according to Qt documentation), but if it's a Qt::QueuedConnection then something like an event is generated which is processed within the receiver's event loop when it is ready. About Qt's implementation of signals/slots I know virtually nothing.


Understand that the QThread class by itself is NOT a different thread.

Right. I probably haven't had this clear in my mind. Though moveToThread(this) does kind of let you get away with not making that distinction--which is probably why I've seen you discourage that here and in other places wysota. Thinking about it more, it does seem like my way of doing it is conceptually mistaken or at least sub-optimal, compared to what you guys have described here. But basically on the new way, all of the methods and data members currently in my QThread subclass will go into the QObject subclass you described in 2). Googling moveToThread(this), the top post is a former QtCentre thread of mine asking about this very issue--Lol!! That's where the technique was suggested to me in the first place. NB, this guy

http://thesmithfam.org/blog/2009/09/30/lock-free-multi-threading-in-qt/

does moveToThread(this), but this guy

http://labs.qt.nokia.com/2010/06/17/youre-doing-it-wrong/

hates it for what sounds like the same reasons.

Anyway, none of this answers my initial question but I guess after I reorganize things that won't matter.

Matt

[wysota]

Anyway, none of this answers my initial question but I guess after I reorganize things that won't matter.
You provided too little information to see where the double connection occurs.

[MattPhillips]

So.... I rewrote my code along the lines described by tbscope. The result is that now the object I moved to the QThread, MyObject, does not process events when the main gui thread has control. I've verified that MyObject is indeed owned by the QThread, and in addition they do non-event-processing related processing in parallel. My code in the gui window constructor is this:


QThread* thd = new QThread();
my_object = new MyObject();
my_object->moveToThread(thd);
thd->start();


All the signals and slots I do in the MyObject constructor, but I get the same thing when they are connected in this constructor. The order of the moveToThread, start() statements doesn't matter. I guess I haven't yet found the right way to do this, any ideas on what is? Thanks--

Edit: The events in question are readyRead() signals generated over a QLocalSocket connection. The QLocalSocket, my_local_socket, is created within MyObject(). However, even after making my_local_socket a child of the QThread, and calling my_local_socket->moveToThread(my_thread), the problem persists.

Matt

[MarekR22]

Check this article about QThread: "You’re doing it wrong… (http://labs.qt.nokia.com/2010/06/17/youre-doing-it-wrong/)"

[wysota]

Show us how you establish your signal/slot connections.

[MattPhillips]

Sweet mother of Jesus, I figured it out. My problems were that a) I didn't at first realize that when you move a class instance to a new thread, you don't move its members, that has to be done separately, and b) you can't use moveToThread at all on a QLocalSocket. According to the warning message I got, my QLocalSocket instance had children, and you can't move an object with children. However I never created an object with my_local_socket as a parent, nor did I see such in the corresponding moc file. As a result QLocalSocket stayed in the gui thread, and I got the results I did. Here's code that actually works (knock on wood):



//include file declaring MyObject

class MainWindow : public QGLWidget //The fact that I'm using GL here is not important
{
Q_OBJECT

MainWindow(QGLWidget* parent)
...

signals:
void ConnectSocket();
...

public slots:
//... various slots
...

private:
MyObject* my_object;
};

MainWindow::MainWindow
{
QThread* thd = new QThread();
my_object = new MyObject(this); //main_window is not the parent of my_object, it's a receiver
my_object->moveToThread(thd);
cerr << "GUI thread: " << thread() << endl;
cerr << "Object thread: " << thd << endl;
cerr << "my_object thread: " << my_object->thread() << endl;
thd->start();

connect(this, SIGNAL(ConnectSocket()), my_object, SLOT(ConnectSocket()), Qt::QueuedConnection);
//I think this maneuver is the *only* way to ensure the QLocalSocket instance is in the worker thread.
emit ConnectSocket();
}


class MyObject : public QObject
{
Q_OBJECT

public:
MyObject(MainWindow* _gui);

signals:
...
//various signals sent to the MainWindow instance

public slots:
void ConnectSocket();
void ReadyRead();
void LostConnection();
void SocketError(QLocalSocket::LocalSocketError err);

private:
MainWindow* gui;
QLocalSocket* my_local_socket;
};

MyObject::MyObject(MainWindow* _gui) : gui(_gui)
{
}
void MyObject::ConnectSocket()
{
bool b(true);
my_local_socket = new QLocalSocket();
my_local_socket->connectToServer("my_local_server", QLocalSocket::ReadOnly);
cerr << "my_local_socket thread: " <<my_local_socket->thread() << endl;
b &= connect(my_local_socket, SIGNAL(readyRead()), this, SLOT(ReadyRead()));
b &= connect(my_local_socket ,SIGNAL(error(QLocalSocket::LocalSocketError)),thi s, SLOT(SocketError(QLocalSocket::LocalSocketError))) ;

b &= connect(this,SIGNAL(my_object_signal()), gui, SLOT(gui_slot()), Qt::QueuedConnection);
// ... and several more of these

cerr << "Connect success, MyObject::ConnectSocket: " << b << endl;
}

The cerr statements output what they should; my_object and my_local_socket are in the worker thread, not the gui thread, and the connection is a success.

Matt

[high_flyer]

a) I didn't at first realize that when you move a class instance to a new thread, you don't move its members, that has to be done separately
Where do you get this from?

@Witek - can you confirm this?

[wysota]

@Witek - can you confirm this?
Not really, it's quite the opposite. You can't move an object that has a parent in different thread. Moving an object that has no parent but it has children is fine. But they have to be children as in parent-child relationship, not as in class member relationship (in other words you have to have a "QObject" relationship between the object and its members).

[MattPhillips]

Hi,

Sorry this is so late--but ok, perhapds it's not strictly accurate to say that the members aren't moved. For example in the case of pointer members, the pointers themselves may be moved (if pointers even have a thread affinity--don't know), but the objects *pointed to*--which is generally what you care about--are not. It's analogous to a shallow copy. To illustrate, here is a version of my code with more cerr statements in it. MyObject::sock is a QLocalSocket, defined in the MyObject constructor.


MainWindow::MainWindow()
{
...
QThread* thd = new QThread();
my_object = new MyObject(this);

cerr << "GUI thread: " << thread() << endl;
cerr << "Object thread: " << thd << endl;
cerr << "Before move: " << endl;
cerr << "my_object thread affinity: " << my_object->thread() << endl;
cerr << "my_object socket thread affinity: " << my_object->sock->thread() << endl;

my_object->moveToThread(thd);
thd->start();

cerr << "After move: " << endl;
cerr << "my_object thread affinity: " << my_object->thread() << endl;
cerr << "my_object socket thread affinity: " << my_object->sock->thread() << endl;
}

The output is as follows:


GUI thread: 0x650f90
Object thread: 0x87b850

Before move:
my_object thread affinity: 0x650f90
my_object socket thread affinity: 0x650f90

After move:
my_object thread affinity: 0x87b850
my_object socket thread affinity: 0x650f90

So, as this example makes clear, when moveToThread is used to change the thread affinity of an object, its pointer members retain the thread affinity they had prior to the move. wysota, I wasn't referring to the parent/child relationship with a) so I don't think we're necessarily disagreeing about anything.

Best,
Matt

[wysota]

You can't prove anything without showing how you create your socket object. I still claim the whole tree of objects is moved to the new thread. If you say something about pointers then you probably don't really understand what thread affinity in Qt is.

[MattPhillips]

wysota, it was


class MyObject : public QObject
{
Q_OBJECT

public:
MyObject(...);

...

QLocalSocket* sock;
};

MyObject::MyObject(...)
{
sock = new QLocalSocket();
}

But, after reading your comment I tried it with

sock = new QLocalSocket(this);

and indeed, like you said, sock *did* pick up the thread affinity of the parent. So it looks like the ConnectSignal workaround I came up with earlier wasn't necessary. So, thanks indeed for the insight and better solution. But it's still the case that *unless* you make a class member the child of the class instance, then member thread affinity is not changed when class affinity is, no?

Matt

[wysota]

Of course the member affinity is not changed. Why would it be? Besides, the socket object is not a member of your class. Only a pointer to a socket object is and that does not have thread affinity.