memory leak with sockets and map

Question

I have a socket server, everytime a new connection is made, a XClient class is instantiated and I am inserting it into a map. I am watching the memory usage through task manager. everytime a new connection is made, lets assume, the memory usage of my program increases by 800kb for example. Inside that class, there is a connected variable, which will tell me wheter this client is active or not. I created a thread to run endlessly and iterate through all the elements of my map and I'm checking if the connected variable is true or false. if it is false, I am (at least I think I am...) releasing the memory used by the previously instantiated XClient class. BUT, the memory usage is being decreased only half of the 800kb (for example, no precise values). So, when a client connects: +800kb. when client disconnects: -400kb. I think I have a memory leak? If I have 100 clients connected, that 400kb that is not being released would turn into 4000kb of non-used(?) memory, and that would be a problem.

So, here is my code. The thread to iterate through all elements:

DWORD Update(XSockets *sockets)
{
while(true)
{
    for(sockets->it = sockets->clients.begin(); sockets->it != sockets->clients.end(); sockets->it++)
    {
        int key = (*sockets->it).first;
        if(sockets->clients[key]->connected == false) // remove the client, releasing memory
        {
            delete sockets->clients[key];
        }
    }
    Sleep(100);
}
return true;
}

The code that is adding new XClients instances to my map:

bool XSockets::AcceptConnections()
{
struct sockaddr_in from;

while(true)
{
    try
    {
        int fromLen = sizeof(from);
        SOCKET client = accept(this->loginSocket,(struct sockaddr*)&from,&fromLen);
        if(client != INVALID_SOCKET)
        {
            srand(time(NULL));
            int clientKey = rand();
            XClient* clientClass = new XClient(inet_ntoa(from.sin_addr),clientKey,client);
            this->clients.insert(make_pair(clientKey,clientClass));
        }
        Sleep(100);
    }
    catch(...)
    {
        printf("error accepting incoming connection!\r\n");
        break;
    }
}

closesocket(this->loginSocket);
WSACleanup();

return true;
}

And the declarations:

    map<int,XClient*> clients;
map<int,XClient*>::iterator it;

Answer 1

You've got several problems, but the chief one is that you appear to be sharing a map between threads without any synchronization at all. That can lead to all kinds of trouble.

Answer 2

Are you using c++11 or Boost? To avoid memory leak nightmares like this, you could create a map of shared pointers. This way, you can let the structure clean itself up.

This is how I would do it:

#include <memory>
#include <map>
#include <algorithm>
#include <functional>
#include <mutex>

typedef std::shared_ptr<XClient> XClientPtr;
std::map<int, XClientPtr> client;
std::mutex the_lock;

bool XSockets::AcceptConnections()
{
/* snip */

    auto clientClass = std::make_shared<XClient>(/*... params ...*/);
    the_lock.lock();
    clients[clientKey] = clientClass;
    the_lock.unlock();
/* snip */
}

bool client_is_connected(const std::pair<int, XClientPtr> &p) {
    return p.second->connected;
}

DWORD Update(XSockets *sockets) {
    while(true) { /* You should probably have some kind of
                     exit condition here. Like a global "running" bool
                     so that the thread will eventually stop. */
        the_lock.lock();

        auto it = sockets->clients.begin(), end = sockets->clients.end();
        for(; it != end; ) {
            if (!it->second->connected)
                //Clients will be destructed here if their refcount goes to 0
                sockets->clients.erase(it++); 
            else
                ++it;
        }
        the_lock.unlock();
        Sleep(100);
    }
    return 1;
}

Note: Above code is untested. I haven't even tried to compile it.

Answer 3

See What happens to an STL iterator after erasing it in VS, UNIX/Linux?. In your case, you are not deleting everything, so you will want to not use a for loop.

sockets->it = sockets->clients.begin();
while (sockets->it != sockets->clients.end())
{
    int key = (*sockets->it).first;
    if(sockets->clients[key]->connected == false) // remove the client, releasing memory
    {
        delete sockets->clients[key];
        sockets->clients.erase(sockets->it++);
    }
    else
    {
        sockets->it++;
    }
}