PDA

View Full Version : QList::erase method does not work if I include an "emit signal" instruction



ClintEastwood
30th April 2013, 23:43
I have the following QList:



QList<PendingMsg> list[MAX_CLIENTS];


And I want to erase some elements from it when a specific time has passed (a typical timeout). Every time an element is removed a signal is emitted, however I don't know why, but the code in charge of erasing the elements does not work when it uses the signal I have defined to do this. The iterator does not iterate the list properly (for example if there is only one element, the for loop runs for 3 iterations, instead of 1, and then a memory access violation is thrown), however, f I only comment the "emit signal" line, everything works...

This is the code in charge of deleting the elements of the list:



void PendingList::checkTimeouts()
{
qint64 t1 = QDateTime::currentMSecsSinceEpoch();
for (int i = 0; i < MAX_CLIENTS; i++)
{
QList<PendingMsg>::iterator it;
for (it = list[i].begin(); it != list[i].end(); )
{
quint64 t0 = it->request.getTimeStamp();
quint64 dift = t1 - t0;
if (dift >= it->timeout)
{
CMD cmd1 = it->cmd;
CMD cmd2 = it->request.getCmd();
it = list[i].erase(it);
emitTimeout((System)i, cmd1, cmd2);
}
else
it++;
}
}
stopIfEmpty();
}


The emitTimeout function emits a signal depending on the parameters cmd1 and cmd2. If I comment this line, everything works..., moreover if I put there for example "emit timeout_ack_unit_ctrl(System)" where timeout_ack_unit_ctrl(System) is a signal declared, it does not work, on the other hand, if I try with the signal "timeout(System)" which is another signal declared, it does work... Can anyone tell me what's happening???

You can see below the whole class:

Header file:



#pragma once

#include "MM.h"
#include "Message.h"
#include <QTimer>
#include <QObject>

struct PendingMsg
{
Message request; //message pending to receive a response
CMD cmd; // type of the pending response {ACK or ORD_RESP}
int timeout; // Time that can be stored the message before emitting a timeout signal

PendingMsg(Message m, CMD c, int t)
{
request = m;
cmd = c;
timeout = t;
};

QString display() const
{
return "[" + cmdTags[cmd] + " (" + cmdTags[request.getCmd()] + ")]";
};
};

class PendingList : public QObject
{
Q_OBJECT

public:
PendingList(QObject* parent, int time);
~PendingList(void);

bool add(System sys, Message req, CMD cmd, int time);
bool remove(System sys, Message msg);

void setCheckTimeInterval(int t) { checkTimeInterval = t;};

int size() const;
void display() const;

bool resumeTimer(){return false;};
bool pauseTimer(){return false;};

private:

int checkTimeInterval;
QTimer timer;

QList<PendingMsg> list[MAX_CLIENTS];

// finds the position of the command cmd inside list[sys]
int match(System sys, Message msg) const;

bool startIfEmpty();
bool stopIfEmpty();

// Reports that the system 'sys' has not sent a message of type 'cmd1' as answer of a message of type 'cmd2'
void emitTimeout(System sys, CMD cmd1, CMD cmd2);

void emitMatch(System sys, Message msg, Message req);

private slots:

void checkTimeouts();

signals:

void timeout(System);
void timeout_ack_unit_ctrl(System);
void timeout_ack_insp_mis(System);
void timeout_ack_mosaic(System);
void timeout_ack_weed_map(System);
void timeout_ack_tre_mis(System);
void timeout_ack_pause(System);
void timeout_ack_stop(System);
void timeout_ack_resume(System);
void timeout_resp_insp_mis(System);
void timeout_resp_mosaic(System);
void timeout_resp_weed_map(System);
void timeout_resp_tre_mis(System);
void timeout_resp_pause(System);
void timeout_resp_stop(System);
void timeout_resp_resume(System);

void ack_insp_mis(System);
void ack_mosaic(System);
void ack_weed_map(System);
void ack_tre_mis(System);
void ack_unit_ctrl(System);
void ack_stop(System);
void ack_resume(System);
void ack_pause(System);

void resp_insp_mis_ok(System);
void resp_mosaic_ok(System);
void resp_weed_map_ok(System);
void resp_tre_mis_ok(System);
void resp_stop_ok(System);
void resp_resume_ok(System);
void resp_pause_ok(System);

void resp_insp_mis_ko(System);
void resp_mosaic_ko(System);
void resp_weed_map_ko(System);
void resp_tre_mis_ko(System);
void resp_stop_ko(System);
void resp_resume_ko(System);
void resp_pause_ko(System);
};


Cpp file:



#include "PendingList.h"

PendingList::PendingList(QObject *parent, int time)
{
this->setParent(parent);
checkTimeInterval = time;
connect(&timer, SIGNAL(timeout()), this, SLOT(checkTimeouts()));
}

PendingList::~PendingList(void)
{
}

bool PendingList::startIfEmpty()
{
if (this->size() > 0 && !timer.isActive()){
timer.start(checkTimeInterval);
return true;
}
return false;
}

bool PendingList::stopIfEmpty()
{
if (this->size() == 0 && timer.isActive()){
timer.stop();
return true;
}
return false;
}

void PendingList::display() const
{
printInfo(0, 0, false, false, "Pending List:");
for (int i = 0; i < MAX_CLIENTS; i++)
{
int n = list[i].size();
if (n > 0)
{
QString line = " - " + sysNames[i] + ": ";
for (int j = 0; j < n; j++)
{
line += list[i][j].display();
}
printInfo(0, 0, false, false, line);
}
}
}

int PendingList::match(System sys, Message msg) const
{
int psys = int(sys);
int n = list[psys].size();
for (int i = 0; i < n; i++)
{
CMD stCmd = list[psys][i].cmd;
CMD cmd = msg.getCmd();
if ((list[psys][i].cmd == msg.getCmd()) &&
((msg.getCmd() == ACK) || (msg.getCmd() == RESP && (msg.getPldByte(0) == list[psys][i].request.getCmd()))))
return i;
}
return -1;
}

bool PendingList::add(System sys, Message req, CMD cmd, int time)
{
quint8 ba[] = {req.getCmd()};
Message msg(cmd, ba, 1);
int p = match(sys, msg);
if (p == -1)
{
int psys = int(sys);
PendingMsg pm(req, cmd, time);
list[psys].append(pm);

startIfEmpty();
return true;
}
return false;
}

bool PendingList::remove(System sys, Message msg)
{
int pos = this->match(sys, msg);
if (pos < 0)
return false;
emitMatch(sys, msg, list[sys][pos].request);
list[sys].removeAt(pos);
stopIfEmpty();
return true;
}

int PendingList::size() const
{
int n = 0;
for (int i = 0; i < MAX_CLIENTS; i++)
n = n + list[i].size();
return n;
}

void PendingList::checkTimeouts()
{
qint64 t1 = QDateTime::currentMSecsSinceEpoch();
for (int i = 0; i < MAX_CLIENTS; i++)
{
QList<PendingMsg>::iterator it;
for (it = list[i].begin(); it != list[i].end(); )
{
quint64 t0 = it->request.getTimeStamp();
quint64 dift = t1 - t0;
if (dift >= it->timeout)
{
CMD cmd1 = it->cmd;
CMD cmd2 = it->request.getCmd();
it = list[i].erase(it);
emitTimeout((System)i, cmd1, cmd2);
}
else
it++;
}
}
stopIfEmpty();
}

void PendingList::emitTimeout(System sys, CMD cmd1, CMD cmd2)
{
printInfo(1, 1, true, true, "Timeout: Pending message (" + cmdTags[cmd1] + " from " + sysNames[sys] + ") erased from the pending messages list");
if (cmd1 == ACK)
switch (cmd2
{
case INSP_MIS:
emit timeout_ack_insp_mis(sys);
break;
case MOSAIC:
emit timeout_ack_mosaic(sys);
break;
case WEED_MAP:
emit timeout_ack_weed_map(sys);
break;
case TRE_MIS:
emit timeout_ack_tre_mis(sys);
break;
case PAUSE_MIS:
emit timeout_ack_pause(sys);
break;
case STOP_MIS:
emit timeout_ack_stop(sys);
break;
case RES_MIS:
emit timeout_ack_resume(sys);
break;
}
else // ORD_RESP
switch (cmd2)
{
case INSP_MIS:
emit timeout_resp_insp_mis(sys);
break;
case MOSAIC:
emit timeout_resp_mosaic(sys);
break;
case WEED_MAP:
emit timeout_resp_weed_map(sys);
break;
case TRE_MIS:
emit timeout_resp_tre_mis(sys);
break;
case PAUSE_MIS:
emit timeout_resp_pause(sys);
break;
case STOP_MIS:
emit timeout_resp_stop(sys);
break;
case RES_MIS:
emit timeout_resp_resume(sys);
break;
}
}

void PendingList::emitMatch(System sys, Message msg, Message pendMsg)
{
CMD pendCmd = pendMsg.getCmd();
if (msg.getCmd() == ACK)
switch (pendMsg.getCmd())
{
case INSP_MIS:
emit ack_insp_mis(sys);
break;
case MOSAIC:
emit ack_mosaic(sys);
break;
case WEED_MAP:
emit ack_weed_map(sys);
break;
case TRE_MIS:
emit ack_tre_mis(sys);
break;
case PAUSE_MIS:
emit ack_pause(sys);
break;
case STOP_MIS:
emit ack_stop(sys);
break;
case RES_MIS:
emit ack_resume(sys);
break;
default:
if (pendMsg.is_UNIT_CTRL())
emit ack_unit_ctrl(sys);
break;
}
else // ORD_RESP
if (msg.getPldByte(1) == OK)
switch (pendCmd)
{
case INSP_MIS:
emit resp_insp_mis_ok(sys);
break;
case MOSAIC:
emit resp_mosaic_ok(sys);
break;
case WEED_MAP:
emit resp_weed_map_ok(sys);
break;
case TRE_MIS:
emit resp_tre_mis_ok(sys);
break;
case PAUSE_MIS:
emit resp_pause_ok(sys);
break;
case STOP_MIS:
emit resp_stop_ok(sys);
break;
case RES_MIS:
emit resp_resume_ok(sys);
break;
}
else
switch (pendCmd)
{
case INSP_MIS:
emit resp_insp_mis_ko(sys);
break;
case MOSAIC:
emit resp_mosaic_ko(sys);
break;
case WEED_MAP:
emit resp_weed_map_ko(sys);
break;
case TRE_MIS:
emit resp_tre_mis_ko(sys);
break;
case PAUSE_MIS:
emit resp_pause_ko(sys);
break;
case STOP_MIS:
emit resp_stop_ko(sys);
break;
case RES_MIS:
emit resp_resume_ko(sys);
break;
}
}

d_stranz
1st May 2013, 02:25
When you emit a signal, you return control to the Qt event loop. If you do not emit any signal from within checkTimeouts(), then your loops run completely to the end and checkTimeouts() exits before control returns to the event loop. When you emit the signal, then all pending events will be processed, including any new timeouts, which means you could be entering checkTimeouts() recursively. So basically, your list gets completely screwed up.

You can probably verify this by putting some qDebug() statements at the beginning and end of checkTimeouts() and see if you get an "entering checkTimeouts()" and "exiting checkTimeouts()" occurring together in pairs of enter / exit. If you get two enters in a row, you have a second timeout being processed before the first one has finished.

Time to re-think your design. Your basic problem is that it is taking longer to process all of the pending messages than the timeout period, so your timeouts are stacking up. So if your application allows you to ignore extra timeouts, then one easy way to avoid the problem is to keep a bool member variable in your PendingList class:



void PendingList::checkTimeouts()
{

qDebug() << "Entering checkTimeouts()";

if ( !inTimeout )
{
inTimeout = true; // this is a member variable of PendingList; initialize it to false in the constructor

qint64 t1 = QDateTime::currentMSecsSinceEpoch();
for (int i = 0; i < MAX_CLIENTS; i++)
{
QList<PendingMsg>::iterator it;
for (it = list[i].begin(); it != list[i].end(); )
{
quint64 t0 = it->request.getTimeStamp();
quint64 dift = t1 - t0;
if (dift >= it->timeout)
{
CMD cmd1 = it->cmd;
CMD cmd2 = it->request.getCmd();
it = list[i].erase(it);
emitTimeout((System)i, cmd1, cmd2);
}
else
it++;
}
}
stopIfEmpty();

inTimeout = false;
}

qDebug() << "Exiting checkTimeouts()";
}


Another way to do it would be to stop the timer when you enter checkTimeouts() and start it when you exit. Or make it a single-shot, and simply restart it when you exit. That way you can be guaranteed that no new timeouts can be generated while you are processing the first one.

amleto
1st May 2013, 14:05
no one talking about the elephant? Why are you instantiating a raw array of QList? Which voice in your head thought that was a good idea?

d_stranz
1st May 2013, 16:23
no one talking about the elephant?

I thought it better to tackle things one step at a time. On the other hand, there's nothing basically wrong with declaring an array of QList<>. It will simply consist of an array of size MAX_CLIENTS of empty QList<> allocated on the stack. Nothing magic about that, and nothing that requires use of QVector< QList< PendingMsg > > or some other fancier data structure. Each QList<> in the array can be manipulated as if it were a standalone QList<>.

I am not sure about the copy semantics, though. I am not sure if accessing "list[i]" results in the return of a copy or of a reference. If it is a copy, then that would be pretty inefficient.

wysota
1st May 2013, 16:50
When you emit a signal, you return control to the Qt event loop. If you do not emit any signal from within checkTimeouts(), then your loops run completely to the end and checkTimeouts() exits before control returns to the event loop. When you emit the signal, then all pending events will be processed, including any new timeouts

No, that's not true, at least when a single execution thread is involved. The only way that the method is re-entered is that the signal is connected to a slot which calls this method again (e.g. emitTimeout() is connected to checkTimeouts()).

d_stranz
2nd May 2013, 18:06
No, that's not true, at least when a single execution thread is involved. The only way that the method is re-entered is that the signal is connected to a slot which calls this method again (e.g. emitTimeout() is connected to checkTimeouts()).

Yes, of course you're right, and my little qDebug() test would have proven me wrong. Blame it on coffee - either not enough or too much.

So that seems to render my whole premise false; presuming that the OP isn't calling checkTimeouts() recursively, then why *does* emitting the signal cause the list to be improperly processed? Maybe one of the slots is also manipulating the list?

wysota
2nd May 2013, 18:43
then why *does* emitting the signal cause the list to be improperly processed?

We don't know if the list is not processed correctly. We'd need to reproduce this errorneous behaviour which is not possible without a small compilable example. I'd start by disconnecting all the signals emitted from within emitTimeout() to see if the problem persists.

ClintEastwood
3rd May 2013, 18:03
Hello all, sorry for the delay, I couldn't test my code until today.



You can probably verify this by putting some qDebug() statements at the beginning and end of checkTimeouts() and see if you get an "entering checkTimeouts()" and "exiting checkTimeouts()" occurring together in pairs of enter / exit. If you get two enters in a row, you have a second timeout being processed before the first one has finished.


I put the qDebug() statements and I got pairs enter / exit, so I wasn't receiving any secondary timeout.



no one talking about the elephant? Why are you instantiating a raw array of QList? Which voice in your head thought that was a good idea?


My system has always the same number (MAX_CLIENTS) of connections to the same number of systems, so I don't need a vector or anything else which can increase/decrease dinamically, I just need an array with MAX_CLIENTS cells.



The only way that the method is re-entered is that the signal is connected to a slot which calls this method again (e.g. emitTimeout() is connected to checkTimeouts()).


Indeed that was the problem. emitTimeout() wasn't connected directley to checkTimeouts() but it was connected to a slot which added a new item in the pendingList structure and then, the timer started again as well as checkTimeouts(). This kind of infinite loop ended up crashing my program and I think getting a bad behaviour when I debugged the program to see how it was processing the list (this point is not very clear to me because I don't know very well how Qt internally manages the signals). Once I deleted the insertion of the new element when the timeout arised everything went ok.

Thank you very much for your help and sorry for the inconveniences caused.

Added after 1 42 minutes:

I have discovered another error which generated the bad behaviour when processing the list after the timeout was emitted. The timeout signal launched several slots and in a row and, inside of one of them, I called the QList::clear method, that's why, when the control returned to the checkTimeout method, the iterator wasn't pointing to a valid address.