removeAll not working

[cristianbdg]

I am developing a server application that receives commands from client applications through TCP. To create the server, I derived a class called MyTcpServer from QTcpServer and in this class I reimplemented the QTcpServer::incomingConnection( int socketDescriptor ) method so that it creates a MyTcpSocket (derived from QTcpSocket) and adds a pointer to this object to a QList. The reason I store a list of all the MyTcpSockets is that I sometimes need to send a message to all clients.

Here's the significant part of my MyTcpServer class:

class MyTcpServer : public QTcpServer
{
Q_OBJECT
 
   public:
 
      MyTcpServer( QObject* parent, const QHostAddress& address, quint16 port )
         : QTcpServer( parent )
      {
         listen( address, port );
      }
 
      QList<MyTcpConnection*> clients()
      {
         return _clients;
      }
 
   protected:
 
      void incomingConnection( int socketDescriptor )
      {
         _clients.append( new MyTcpConnection( this, socketDescriptor ) );
      }
 
   private:
 
      QList<MyTcpConnection*> _clients;
 
};

class MyTcpServer : public QTcpServer
{
Q_OBJECT
      
   public:

      MyTcpServer( QObject* parent, const QHostAddress& address, quint16 port )
         : QTcpServer( parent )
      {
         listen( address, port );
      }
      
      QList<MyTcpConnection*> clients()
      {
         return _clients;
      }
      
   protected:

      void incomingConnection( int socketDescriptor )
      {
         _clients.append( new MyTcpConnection( this, socketDescriptor ) );
      }

   private:

      QList<MyTcpConnection*> _clients;

};

To copy to clipboard, switch view to plain text mode 

This part seems to work perfectly, but if someone sees something wrong, feel free to say it to me.

The part that doesn't work well is when a client disconnects. Like the fortuneserver/client example, I connected the connectionClosed() signal to the deleteLater() slot, but I also need to delete the pointer from the _clients list in the MyTcpServer class. To do that, I store a pointer to the MyTcpServer so I can get the clients list and delete the pointer to the MyTcpConnection using QList::removeAll( const T& value ).

The significant code:

class MyTcpConnection : public QTcpSocket
{
Q_OBJECT
 
   public:
 
      MyTcpConnection( MyTcpServer* parent, int sock )
         : QTcpSocket( static_cast<QObject*>( parent ) ),
      {
         _server = parent;
         connect( this, SIGNAL( disconnected() ), SLOT( slotOnDisconnect() ) );
         connect( this, SIGNAL( connectionClosed() ), SLOT( deleteLater() ) );
         setSocketDescriptor( sock, QAbstractSocket::ConnectedState, QIODevice::ReadWrite );
      }
 
   private:
 
      MyTcpServer* _server;
 
   private slots:
 
      void slotOnDisconnect()
      {
         _server->clients().removeAll( this );
      }
 
};

class MyTcpConnection : public QTcpSocket
{
Q_OBJECT

   public:

      MyTcpConnection( MyTcpServer* parent, int sock )
         : QTcpSocket( static_cast<QObject*>( parent ) ),
      {
         _server = parent;
         connect( this, SIGNAL( disconnected() ), SLOT( slotOnDisconnect() ) );
         connect( this, SIGNAL( connectionClosed() ), SLOT( deleteLater() ) );
         setSocketDescriptor( sock, QAbstractSocket::ConnectedState, QIODevice::ReadWrite );
      }

   private:

      MyTcpServer* _server;

   private slots:

      void slotOnDisconnect()
      {
         _server->clients().removeAll( this );
      }

};

To copy to clipboard, switch view to plain text mode 

The problem is that removeAll doesn't remove the MyTcpConnection pointer from the _clients list!

Making the function a bit more verbose, like this:

qDebug( "before: %d clients", _server->clients().size() );
int removed = _server->clients().removeAll( this );
qDebug( "%d clients removed", removed );
qDebug( "after: %d clients", _server->clients().size() );

qDebug( "before: %d clients", _server->clients().size() );
int removed = _server->clients().removeAll( this );
qDebug( "%d clients removed", removed );
qDebug( "after: %d clients", _server->clients().size() );

To copy to clipboard, switch view to plain text mode 

...this is what happens:
before: 2 clients
1 clients removed
after: 2 clients

I thought that maybe removeAll didn't really find and removed the client although it returns 1, so I also tried this:

qDebug( "before: %d clients", _server->clients().size() );
for( int i = 0; i < _server->clients().size(); ++i )
{
   MyTcpSocket* client = _server->clients().at(i);
   if( client == this )
   {
      qDebug( "FOUND IT: removing..." );
      _server->clients().removeAt(i);
      break;
   }
   else qDebug( "this is not the one" );
}
qDebug( "after: %d clients", _server->clients().size() );

qDebug( "before: %d clients", _server->clients().size() );
for( int i = 0; i < _server->clients().size(); ++i )
{
   MyTcpSocket* client = _server->clients().at(i);
   if( client == this )
   {
      qDebug( "FOUND IT: removing..." );
      _server->clients().removeAt(i);
      break;
   }
   else qDebug( "this is not the one" );
}
qDebug( "after: %d clients", _server->clients().size() );

To copy to clipboard, switch view to plain text mode 

...but no luck:
before: 2 clients
FOUND IT: removing...
after: 2 clients

Does anyone know what I'm doing wrong?

P.S. Sorry for the long explanation, but I explained my problem in another place and I got no answers, so I thought trying again here, with more information. I hope the cross-posting isn't a problem as I didn't get a single answer at the other place.

[wysota]

Are you sure removeAll actually gets called? Could you put a qDebug statement inside the slot which does the removal? If removeAll() returns 1, then it did remove the item, maybe (because of some optimisations?) you get an invalid value for size of the list afterwards? Could you, after removal, iterate the list and see what elements it really contains?

[cristianbdg]

Thanks for the incredibly quick reply!

> Are you sure removeAll actually gets called? Could you put a qDebug statement inside the slot which does the removal?

The qDebug statements I mentioned are actually in the slotOnDisconnect() slot, before and after the removeAll call (I should have made that clearer). So I am pretty sure that removeAll gets called.

> If removeAll() returns 1, then it did remove the item, maybe (because of some optimisations?) you get an invalid value for size of the list afterwards? Could you, after removal, iterate the list and see what elements it really contains?

I will do a little test iterating over the list, but I can tell you already that when I connect a client, close it, and then connect again, the client starts receiving all messages twice. Isn't that strange? Shouldn't I get a segfault for the following reason?

1. I connect a client, a MyTcpSocket object is created and a pointer is stored in the _clients list.
2. I close the client. The connectionClosed() -> deleteLater() connection deletes the MyTcpSocket, but removeAll doesn't remove the pointer from the _clients list.
3. I connect the client again, another MyTcpSocket object is created and its pointer stored in the _clients list. Now we have two items in the list, but one is pointing to a deleted object.
4. When sending a message to the clients a pointer to a deleted object gets used (step 2). Segfault?

It's as if the first MyTcpSocket never gets deleted and when a new client connects, the same socket is reused. Maybe that's causing the problem?

Thanks in advance for your interest

[bbc58206]

hello there..im also working with this fuction..
how can i list all connected client?
can u give me ur working example

[ggrinder]

Hi,

the problem is that you don't remove the client form the "real" list of clients.

Your clients method returns a copy of the clients list, your code would only work if you return a reference.

Changing

QList<MyTcpConnection*> MyTcpServer::clients()

QList<MyTcpConnection*> MyTcpServer::clients()

To copy to clipboard, switch view to plain text mode 

to

QList<MyTcpConnection*>& MyTcpServer::clients()

QList<MyTcpConnection*>& MyTcpServer::clients()

To copy to clipboard, switch view to plain text mode 

should do the trick.