How can I free memory from pointer which is value and argument of object and its constructor?

Question

I am using class Client

class Client
{
     Client(sf::TcpSocket *_socket);
     sf::TcpSocket *socket;
};

Constructor:

Client::Client(sf::TcpSocket *_socket)
{
    socket=_socket;
}

In the thread, my listener is waiting for clients to connect and add them to a vector of clients.

while(isAcceptingConnections==true)
    {
        sf::TcpSocket *socket=new sf::TcpSocket;
        if (listener.accept(*socket) == sf::Socket::Done)
        {
            socket->setBlocking(false);
            Client client(socket);
            clients.push_back(client);
        }
        else
        delete socket;
    }

As you see I am dynamically allocating memory for each new connection and using a pointer to each socket as an argument for Client constructor. I would like to be able to free this memory through Client object once the connection is disabled. How can I do it? In a different thread, I have packet transfer which receives and sends in loop packets to each client. I tried using "delete clients[i].socket" but it crashes the server. Any idea?

What I am trying to do is to be able to free memory once the socket is no more used by the client.

for(unsigned int i=0;i<clients.size();i++)
{
     if(!clients[i].socket->send(heartbeat))
     {
          clients[i].socket->disconnect();
          delete clients[i].socket;
          clients.erase(clients.begin()+i,clients.begin()+i+1);
     }
}

or

for(unsigned int i=0;i<clients.size();i++)
{
     if(!clients[i].socket->send(heartbeat))
     {
          clients.erase(clients.begin()+i,clients.begin()+i+1);
     }
}

~Client()
{
     client.socket->disconnect();
     delete client.socket;
}

Both options are crashing my server though. How should I do it?

You mention that this is threaded; but I see no syncronisation in your code. ; nor do you say where your crash is. Are you *SURE* that it's not a threading issue? Aside, you may wish to read https://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one — UKMonkey, Feb 15 '18 at 10:20
The solution is spelled `std::unique_ptr`. Make the ownership explicit, use the correct type for it. Owning raw pointers are ill-advised. — StoryTeller - Unslander Monica, Feb 15 '18 at 10:20
I am pretty sure that it's not threading issue since deleting client from vector isn't causing crash, just trying to free memory does. Is there other way than unique pointer or should I just try to use it and see how it goes? — Advent, Feb 15 '18 at 10:30
@Advent what's wrong with `unique_ptr`? It's the solution to your problem — john, Feb 15 '18 at 10:37
@Advent Also, your `for` loop looks flawed to me. If you erase an item, you shouldn't increment `i` in that loop. The correct way to do this is to either control the index to not increment itself, or better to use the *erase-remove_if* idiom and drop the `for` loop. — PaulMcKenzie, Feb 15 '18 at 11:14
Yeah, I am quite new in multithreading, not counting program simulating bridges and cars (mutex and threads) I never used it before. I had to use it this time because I can't listen to connections and send packets at same time additionally progress server logic, I can't wait for connection and block whole client (not letting client for example cancel the connection or even draw interface itself). Since I am quite new I have little to no idea how to syncronise them. I am using references to same object and it works as far with no problems or syncro needed at all. — Advent, Feb 15 '18 at 11:35

Advent · Answer 1 · 2018-02-15T11:30:27.657

Actually it happened to be something like that:

When I created in scope my Client to add it to vector, it somehow deleted itself and added at same time (?). I saw that by testing Client destructor.

  sf::TcpSocket *socket=new sf::TcpSocket;
    if (listener.accept(*socket) == sf::Socket::Done)
    {
        std::cout<<"NEW CLIENT ("<<clients.size()+1<<")"<<std::endl;
        socket->setBlocking(false);
        Client client(socket);
        clients.push_back(client);
    }

What I did to solve that problem and and be able to delete socket itself?

Instead of storing vector of Clients I am storing vector of pointers to clients. I create client dynamically using same pointer to socket as well.

sf::TcpSocket *socket=new sf::TcpSocket;
    if (listener.accept(*socket) == sf::Socket::Done)
    {
        std::cout<<"NEW CLIENT ("<<clients.size()+1<<")"<<std::endl;
        socket->setBlocking(false);
        Client *client=new Client(socket);
        clients.push_back(client);
    }

When client disconnects, I delete dynamically allocated memory and erase it from vector.

if(clients[i]->state==ClientState::DISCONNECTED)
{
     delete clients[i];
     clients.erase(clients.begin()+i,clients.begin()+i+1);
}

Now to free memory of socket itself after deleting client, I use Client destructor:

Client::~Client()
{
    std::cout<<"CLIENT DELETED"<<std::endl;
    socket->disconnect();
    delete socket;
}

Also to make sure that I clear all that memory when I close the program, in destructor of objects which uses vector of clients:

ConnectionManager::~ConnectionManager()
{
    for(unsigned int i=0;i<clients.size();i++)
    delete clients[i];
    clients.clear();
}

That works for me, if you have something to add or advice, feel free to.

Your deletion can be made much more safer by using `std::partition`, `std::for_each`, and then `std::erase`. See [this code snippet](http://coliru.stacked-crooked.com/a/8b00b9d148a655a7). Basically all that is done is to move (partition) the items to delete to one part of the vector, delete those items, then erase them from the vector. Exactly as the code snippet says it does. — PaulMcKenzie, Feb 15 '18 at 21:24
I really have a lot to learn. Thanks for advice. I will use that in future though because there is no more time for me and I need to finish my connection protocol as fast as possible. Maybe you could point me a direction or really good read to learn the most important things about C++? — Advent, Feb 16 '18 at 07:51
You can start [here](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). If you're wondering why in your case the partitioning algorithm was used instead of directly calling `std::remove_if`, the `remove_if` function invalidates the entries you're moving, and you don't want them invalidated immediately. You want them to be alive so that a later call to `delete` can be done without error. Of course, using smart pointers would avoid this and simply a `std::remove_if` would have worked without partitioning. — PaulMcKenzie, Feb 16 '18 at 10:52

How can I free memory from pointer which is value and argument of object and its constructor?

1 Answers1