Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Container(list) of TcpSockets not behaving (NonCopyable)  (Read 4990 times)

0 Members and 1 Guest are viewing this topic.

TheRabbit

  • Newbie
  • *
  • Posts: 26
    • View Profile
Container(list) of TcpSockets not behaving (NonCopyable)
« on: November 06, 2013, 05:17:04 am »
My server uses a list of TcpSockets called host:
std::list<sf::TcpSocket> host; //sockets we will use to communicate

When I go to add a new socket, I'm unable to:
host.push_back(sf::TcpSocket());
This code gives me an error regarding the 'NonCopyable' declaration.

Code: [Select]
c:\sfml-2.1-windows-vc11-32bits\sfml-2.1\include\sfml\network\socket.hpp(178): error C2248: 'sf::NonCopyable::NonCopyable' : cannot access private member declared in class 'sf::NonCopyable'
1>          c:\sfml-2.1-windows-vc11-32bits\sfml-2.1\include\sfml\system\noncopyable.hpp(67) : see declaration of 'sf::NonCopyable::NonCopyable'
1>          c:\sfml-2.1-windows-vc11-32bits\sfml-2.1\include\sfml\system\noncopyable.hpp(42) : see declaration of 'sf::NonCopyable'
1>          This diagnostic occurred in the compiler generated function 'sf::Socket::Socket(const sf::Socket &)'

Is there a way around this? Before, I was using a fixed array of TcpSockets:
sf::TcpSocket host[10];
and had no problems, but it wasn't very robust. I'm trying to make the server a bit more robust to allow it to create new connections and delete old ones without going though all of the hoops I went through using a fixed array.

What can I do to get around this?

I've tried making a new class that (for right now) only holds a sf::TcpSocket item in it. But it seems to be an inherent problem with the way that 'push' works on containers, in that it creates a copy of the item before putting it into the list. Since TcpSockets are special and can't be copied (I assume that would cause actual problems and I shouldn't just overload it) I can't add a new one into a list.
« Last Edit: November 06, 2013, 05:41:49 am by TheRabbit »

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #1 on: November 06, 2013, 07:32:24 am »
Store pointers.
Laurent Gomila - SFML developer

TheRabbit

  • Newbie
  • *
  • Posts: 26
    • View Profile
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #2 on: November 06, 2013, 02:53:24 pm »
Thanks for the reply, but I'm not sure I follow. I can't do a `push` using a pointer from what I've read and attempted, and dereferencing the pointer gives me the same issue, noncopyable.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #3 on: November 06, 2013, 03:02:10 pm »
Just make sure you don't copy when dereferencing. Obtaining a reference to the object doesn't require copyability.

One possibility to do it is std::unique_ptr. In contrast to raw pointers, it automatically frees the memory. If you find that the identifiers become too long, use typedef.
std::list<std::unique_ptr<sf::TcpSocket>> host;
...
std::unique_ptr<sf::TcpSocket> socket(new sf::TcpSocket);
host.push_back(std::move(socket));
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

TheRabbit

  • Newbie
  • *
  • Posts: 26
    • View Profile
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #4 on: November 06, 2013, 04:40:19 pm »
Thanks for the reply Nexus. I was thinking that might have been what Laurent meant, but I wasn't sure.

2 quick questions, it seems like unique pointers are smart, in that I don't need to manually clean up after them, so there wouldn't be any additional steps I would need to do when I pop/erase a list element, right?]
 host.erase(*it)
Would handle the erase and cleanup? edit: just remembered I would still need to release it out of the listener, so it isn't totally automated.

Second, why would I do this:
 std::unique_ptr<sf::TcpSocket> socket(new sf::TcpSocket);
host.push_back(std::move(socket));
Instead of:
host.push_back(new *std::unique_ptr<sf::TcpSocket>)

(sorry for the crappy code, doing this from my phone)
« Last Edit: November 06, 2013, 04:52:21 pm by TheRabbit »

zsbzsb

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #5 on: November 06, 2013, 05:01:04 pm »
Second, why would I do this:
 std::unique_ptr<sf::TcpSocket> socket(new sf::TcpSocket);
host.push_back(std::move(socket));
Instead of:
host.push_back(new *std::unique_ptr<sf::TcpSocket>)

Because your "instead of" pushes in a raw pointer to a std::unique_ptr. So in essence you are pushing back a pointer of a pointer and then when your function returns the std::unique_ptr will be destroyed including your socket.
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor

TheRabbit

  • Newbie
  • *
  • Posts: 26
    • View Profile
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #6 on: November 07, 2013, 05:53:28 am »
Just make sure you don't copy when dereferencing. Obtaining a reference to the object doesn't require copyability.

One possibility to do it is std::unique_ptr. In contrast to raw pointers, it automatically frees the memory. If you find that the identifiers become too long, use typedef.
std::list<std::unique_ptr<sf::TcpSocket>> host;
...
std::unique_ptr<sf::TcpSocket> socket(new sf::TcpSocket);
host.push_back(std::move(socket));

Just had a chance to try this and the code written your way works fine, but I can't figure out how to put the socket into a class, everything I've tried either fails at Noncopyable or I get an error about the private member function std::unique_ptr<_Ty>.

I was able to get it working using a regular pointer, but then I was having memory problems because it wasn't letting me delete them...

So here's what the Client class looks like:
class Client
{
public:
        Client();
        ~Client();
        void loadClient(const std::string &clientName); //looks up a user based on their name


public:
        std::unique_ptr<sf::TcpSocket> socket; //socket handling communication to this client
        std::string name; //client's user name
        int ID; //client's unique ID

private:


private:

};
And then running this code:
        std::list<Client> clientList; //sockets we will use to communicate
        clientList.push_back(Client()); //put a new client into the list
Gives this error:
Code: [Select]
1>  main.cpp
1>c:\documents\visual studio 2012\projects\game server\blank sfml\main.cpp(67): error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access private member declared in class 'std::unique_ptr<_Ty>'
1>          with
1>          [
1>              _Ty=sf::TcpSocket
1>          ]
1>          c:\program files (x86)\microsoft visual studio 11.0\vc\include\memory(1447) : see declaration of 'std::unique_ptr<_Ty>::unique_ptr'
1>          with
1>          [
1>              _Ty=sf::TcpSocket
1>          ]
1>          This diagnostic occurred in the compiler generated function 'Client::Client(const Client &)'
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

I tried putting the std::move code into the constructor, but that didn't work either (got the same error).

More help would be appreciated!

Edit: I kind of fixed it. I don't know if this makes any sense, but here's what I did:
class Client
{
public:
        Client();
        Client(const Client &tempC); //added a copy constructor
        ~Client();
        void loadClient(const std::string &clientName); //looks up a user based on their name


public:
        std::unique_ptr<sf::TcpSocket> socket; //socket handling communication to this client
        std::string name; //client's user name
        int ID; //client's unique ID

private:


private:

};

Client::Client()
{
        socket = std::move(std::unique_ptr<sf::TcpSocket> (new sf::TcpSocket));
}

Client::Client(const Client &tempC)
{
        socket = std::move(std::unique_ptr<sf::TcpSocket> (new sf::TcpSocket));
}

Client::~Client()
{
}

int hostConnect()
{
        std::list<Client> clientList; //sockets we will use to communicate
       
        Client newClient;
        clientList.push_back(newClient); //put a new client into the list
        ...
                        if(selector.isReady(listener)) //we have a new incoming connection
                        {
                                Client tempClient;
                                clientList.push_back(Client(tempClient)); //put a new client into the list
                                if (listener.accept(*clientList.back().socket) != sf::Socket::Done)
                                {
                                        printf("Incoming connection failed.\n"); //error handler... kind of
                                        clientList.pop_back(); //remove the failed connection from the list
                                }
                                selector.add(*clientList.back().socket); //add the new connection into the selector listener thing
                                printf(std::string("Remote client " + (*clientList.back().socket).getRemoteAddress().toString() + " has connected to the server.\n").c_str());
                                (*clientList.back().socket).setBlocking(false); //make sure it is non-blocking
                        }
...
                                bool FLAGDC = true; //use this to break out of loops when we get a disconnect
                                for(auto it = clientList.begin(); ((it != clientList.end()) && (FLAGDC));) //cycle through the available sockets
                                {
...
                                                case sf::Socket::Disconnected:
                                                case sf::Socket::Error:
                                                        //lost the connection to this socket
                                                        selector.remove(*(*it).socket); //stop listening to the client that disconnected
                                                        clientList.erase(it);
                                                        printf(std::string("Remote client disconnected.\n").c_str());
                                                        FLAGDC = false;
                                                        break;
...
                                        if (FLAGDC)
                                                it++;
                                        else
                                                break;
 
One problem that I found with my code, and the reason there is that if/else at the very bottom is that if you delete a client in the middle of an auto for loop, it totally breaks the iterator and crashes the program. So instead I made the for loop the way it is.

Tested the server with a few dozen connects and disconnects and had 15 concurrent users at one point, haven't seen any issues at all.
« Last Edit: November 07, 2013, 06:39:04 am by TheRabbit »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #7 on: November 07, 2013, 11:09:06 am »
Just had a chance to try this and the code written your way works fine, but I can't figure out how to put the socket into a class, everything I've tried either fails at Noncopyable or I get an error about the private member function std::unique_ptr<_Ty>.
If the socket is a member of a class, the class itself will become noncopyable. As long as you don't invoke copy constructor or copy assignment operator, this is not a problem.

Edit: I kind of fixed it. I don't know if this makes any sense, but here's what I did:
The question is how your ownership semantics look. Also make sure you consider the rule of three, i.e. also declare copy assignment operator when you need copy semantics. But I think in your case, you don't want to copy the client, but move it to the container. For this, you need C++11 move semantics.

Enabling move semantics requires you to define some special member functions, move constructor and move assignment operator. Actually, a fully C++11 conforming compiler can generate them on its own, if you define none of them (see rule of five). Nevertheless I'll show you how to do it manually, it doesn't hurt to know the syntax:
class Client
{
public:
    // Default constructor
    Client()
    : socket(new sf::TcpSocket)
    {
    }

    // Move constructor
    Client(Client&& source)
    : socket(std::move(source.socket))
    {
    }

    // Move assignment operator
    Client& operator= (Client&& source)
    {
        socket = std::move(source.socket);
        return *this;
    }

    // No destructor necessary

private:
    std::unique_ptr<sf::TcpSocket> socket;
}
These may be new C++ features for you. The && denotes a rvalue reference, it can bind any rvalues (that is, temporary objects and results of std::move()). std::move() itself is only required to convert a named object (lvalue) to a rvalue, in order to bind a rvalue reference to it.

socket = std::move(std::unique_ptr<sf::TcpSocket> (new sf::TcpSocket));
You never need to invoke std::move() on unnamed temporary objects, since they are already rvalues. Instead, the equivalent code would be:
socket.reset(new sf::TcpSocket);

I recommend to read about C++11 move semantics. There was a good article series called C++Next, but it appears to be offline at the moment. Try the web archive.

By the way, your code is very convoluted, you should clean it up. Concerning removal of elements, don't do it while iterating (or at least consider the return value of erase()). Have a look at std::remove_if(). And don't use printf() when you want to output std::string objects, after all C++ provides std::cout.
« Last Edit: November 07, 2013, 11:16:27 am by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

TheRabbit

  • Newbie
  • *
  • Posts: 26
    • View Profile
Re: Container(list) of TcpSockets not behaving (NonCopyable)
« Reply #8 on: November 08, 2013, 03:45:54 am »
Thanks for the pointers!

I read this article: http://www.cprogramming.com/c++11/rvalue-references-and-move-semantics-in-c++11.html and it helped a lot with explaining the new rvalues and the way temporary things work now.

The code is very convoluted because I'm copying and pasting out of other things that I've written, or things written for me (like your code). I'll clean it up once it's working, but my first priority was to get it working.

Thanks again, your posts help a lot.

Here's the final (for now) chat server code:
int hostConnect()
{
        std::list<Client> clientList; //sockets we will use to communicate
        sf::TcpListener listener; //will listen on a specific port for an incoming connection
        sf::Packet dataPacketIn; //data packet used to receive data from a client
        sf::Packet dataPacketOut; //data packet used to transmit data to a client
        sf::SocketSelector selector; //used to handle multiple clients
        unsigned short port=53000; //default port
        std::string str_in; //string used for communication


        if (listener.listen(port) != sf::Socket::Done) {
                std::cout << "Failed to listen to port " + std::to_string(port) + ". Something else already attached to port?\n";
                return 1;}
        else
                std::cout << "Currently listening to port " + std::to_string(port) + ". Waiting for first connection...\n";

        selector.add(listener); //put the new connection listener into our selector
        listener.setBlocking(false); //now that we have our first connection, we no longer want to block when checking for new connections

        while (true) //main connection loop
        {      
                if (selector.wait()) //server sits and waits for an event to occur on the selector
                {
                        if(selector.isReady(listener)) //we have a new incoming connection
                        {
                                clientList.push_back(std::move(Client())); //put a new client into the list
                                if (listener.accept(*clientList.back().socket) != sf::Socket::Done)
                                {
                                        std::cout << "Incoming connection failed.\n"; //error handler... kind of
                                        clientList.pop_back(); //remove the failed connection from the list
                                }
                                else
                                {
                                        selector.add(*clientList.back().socket); //add the new connection into the selector
                                        (*clientList.back().socket).setBlocking(false); //make sure it is non-blocking
                                        std::cout << "Remote client " + (*clientList.back().socket).getRemoteAddress().toString() + " has connected to the server.\n";
                                }
                        }
                        else //if it wasn't an incoming connection it was either an incoming message or a disconnect
                        {
                                dataPacketIn.clear(); //empty our data packet to prepare it for receives
                                bool FLAGDC = true; //use this to break out of loops when we get a disconnect
                                for(auto it = clientList.begin(); ((it != clientList.end()) && (FLAGDC));) //cycle through the available sockets, or break when we hit a DC'd client
                                {
                                        if(selector.isReady(*(*it).socket))
                                        {
                                                switch ((*(*it).socket).receive(dataPacketIn))
                                                {
                                                case sf::Socket::NotReady:
                                                        //this is not the socket you were looking for
                                                        break;
                                                case sf::Socket::Disconnected:
                                                case sf::Socket::Error:
                                                        //lost the connection to this socket
                                                        selector.remove(*(*it).socket); //stop listening to the client that disconnected
                                                        clientList.erase(it); //pop the DC'd client out of the list
                                                        std::cout << "Remote client disconnected.\n";
                                                        FLAGDC = false; //set our breakout flag
                                                        break;
                                                case sf::Socket::Done:
                                                        //a socket is sending us a message!
                                                        dataPacketIn >> str_in; //load the packet into the string
                                                        std::cout << str_in  + "\n"; //print it on the server
                                                        dataPacketOut.clear(); //load the string into a packet to send out
                                                        dataPacketOut << str_in;
                                                        for(auto it2 = clientList.begin(); it2 != clientList.end(); it2++)
                                                                if ((*(*it2).socket).send(dataPacketOut) != sf::Socket::Done) //error catcher... kind of
                                                                        std::cout << "ERROR!! Failed to send data to client!\n";
                                                        break;
                                                }
                                        } //end if(selector.isReady(*it))
                                        if (FLAGDC) //if we didn't hit any dead clients
                                                it++;
                                        else //if we did hit a dead client our iterator is invalid and we need to start the 'for' over again
                                                break;
                                } //end for(auto it = clientList.begin(); it != clientList.end(); it++) //cycle through the available sockets
                        }//end else //if it wasn't an incoming connection it was either an incoming message or a disconnect
                }//selector's if loop
        }//end main while loop
    return 0; //should theoretically never get here...
}
Maybe it will help someone else out.
« Last Edit: November 08, 2013, 04:10:27 am by TheRabbit »