Ytukyg

I'm creating a very simple client/server application.
I want my server to detect how many clients have connected and if there are more than 2, just print a message.

The code for the server I have so far is very minimal:

std::vector<sf::TcpSocket*> clients;



sf::TcpListener listener;



if (listener.listen(SERVERPORT) != sf::Socket::Done)

{

    printf("Errorn");

}



printf("Waiting for first connection...n");



sf::TcpSocket *socket = new sf::TcpSocket;



if (listener.accept(*socket) != sf::Socket::Done)

{

    printf("Errorn");

}



clients.push_back(socket);



while (clients.size() < 2)

{

    printf("Waiting for second connection...n");

}

The problem I have is that it detects the first connection with no problem, but it doesn't detect the second one, even though my second client is connected. For the client connection I'm just using the very simple code explained in the SFML documentation.
I'm very confused as clients.size() always returns 1.

Because you don't accept a second connection...

Changing code order a little:

std::vector<sf::TcpSocket*> clients;



sf::TcpListener listener;



if (listener.listen(SERVERPORT) != sf::Socket::Done)

{

    printf("Errorn");

}



printf("Waiting for first connection...n");



while (clients.size() < 2)

{

    sf::TcpSocket *socket = new sf::TcpSocket;

    if (listener.accept(*socket) != sf::Socket::Done)

    {

        printf("Errorn");

    }



    clients.push_back(socket);



    // OK, that one would be printed twice!!!

    printf("Waiting for second connection...n");

}

This won't be the final loop, though, it is just intended for demonstration.

Be aware that you don't delete the clients created anywhere (-> memory leak!). You might consider usage of smart pointers instead of raw pointers.

OK, so now: you want to accept arbitrary number of clients, and as soon as number you have more than two clients, run some application.

At very first, you need to continue accepting new clients on the loop (I changed the logging messages a little...:

std::vector<sf::TcpSocket*> clients;

sf::TcpListener listener;

if (listener.listen(SERVERPORT) != sf::Socket::Done)

{

    printf("Errorn");

    return; // no need to go on on error!

}



printf("start waiting for clients...n");



for(;;) // endless loop...

{

    sf::TcpSocket* socket = new sf::TcpSocket;

    if (listener.accept(*socket) != sf::Socket::Done)

    {

        printf("Errorn");

        // TODO: consider some appropriate error handling

        // minimally:

        delete socket; // otherwise, you produce memory leaks!

    }



    clients.push_back(socket);

    printf

    (

        "new client accepted; number of clients now: %llun"

        "continue waiting...n",

        clients.size()

    );

    // llu: most likely on 64-bit machine, on 32 bit either lu or u,

    // depending on type of size_t; you get around having to decide

    // if using C++ streams instead...

}

Problem now is: You additionally need to handle the connections appropriately! Have a look at this SFML tutorial, especially the section Blocking on a group of sockets. The selector described there is exactly what you need. Most important:

A selector can monitor all types of sockets: sf::TcpSocket, sf::UdpSocket, and sf::TcpListener.

So you can add all of your sockets and the listener to. You still have to maintain your instances yourself, though:

A selector is not a socket container. It only references (points to) the sockets that you add, it doesn't store them. There is no way to retrieve or count the sockets that you put inside. Instead, [...].

Well, you covered the 'instead' part already with your vector. Normally I'd recommend storing the objects in the vector directly; however, you cannot, as you need references to in the selector. So you need to store pointers in the vector – or you can use a std::list instead, which won't invalidate references or pointers to stored data on insertion or deletion of objects (apart from pointers to the removed object itself, of course).

If you want to stay with the vector, you might consider using smart pointers, though, so that you don't have to care for memory management (e. g. std::vector<std::unique_ptr<sf::TcpSocket>> clients).

I personally would prefer the std::list:

std::list<sf::TcpSocket> clients; // notice: no pointers...

sf::TcpListener listener;

sf::SocketSelector selector;

selector.add(listener); // you want to listen as well...

for(;;)

{

    // wait on selector as described in tutorial, I'd recommend only

    // waiting for one second: you might want to handle the application

    // exiting at some point of time!

}

If waiting was successful, you'd now first check the server socket, then the clients):

if(listener.isReady())

{

    // OK, we now know that a client is waiting, so next steps won't block:

    clients.emplace_back(); // new client

    if(listener.accept(clients.back()) != sf::Socket::Done)

    {

        // OK, something went wrong, current client us valueless...

        clients.pop_back();

        // adding and then removing? well, normally, this case here

        // should not occur anyway, so we optimized for the far more

        // common case.

    }

    else

    {

        // want to listen on as well

        selector.add(clients.back());

    }

}

for(auto& c : clients)

{

    if(c.isReady())

    {

        // handle communication

    }

}

Handling communinication is up to you... I wouldn't just communicate if at least two clients are connected. If you leave the messages from one single client unhandled, you might end up in needing to handle quite a bunch of outdated messages as soon as another client arrives, so I'd rather handle anything incoming immediately even with one single client. If it appears more appropriate to you, you might still place a if(clients.size() >= 2) around the (range based) for loop.

If a client disconnects, don't forget to remove it from the selector! Having done so, you can safely remove it from the clients list as well.

Finally: the endless loop!

You might want to replace this via some conditional loop, checking if the server should still be running at all or not. You'd then yet need some mechanism to set this condition to false. There are different options for, e. g. some separate thread checking some specific input, signals (especially on POSIX systems), ...