Advertisement

Issue With Simple Client To Server Chat Program

Started by November 17, 2014 09:45 PM
28 comments, last by Psychopathetica 10 years, 2 months ago

Using select is easy.

Using async is a little more complicated and OS specific.

Here is a basic select() example polling a single socket and returning immediately


 
fd_set fd;
timeval tv;
int r;
 
FD_SET( my_socket, &fd );
tv.tv_sec = tv.tv_usec = 0;
 
r = select( my_socket + 1, &fd, 0, 0, &tv );
if( r < 1 )
you have nothing for recv()
else
call recv()
 

You can use the same code for your accept socket so it doesn't block until someone tries to connect.

A more efficient use of select() would be to use multiple sockets in the fd_set and use FD_ISSET() to see which ones had data to recv on.

You may have to code a loop that builds fd_sets and calls select() because an fd_set has a limited maximum size.

Basically, you add a lot of sockets to the fd_set using FD_SET(), call select() checking the return to see if you had any hits, then test every socket using FD_ISSET() and calling recv() or accept() on the sockets that test positive.

You can also use select() as a replacement for Sleep() because it has a wait timer (it waits a maximum of timeval unless something hits a socket then it returns immediatly) which will also reduce latency because the clients don't have to wait on your server to "wakeup" from Sleep() to get a response..

For a proper production system a async socket system is needed. This usually means async dns too. To do this you are best abstracting the socket calls and writing a socket engine to handle the i/o events. This is something I have a fair bit of knowledge on in a non gamedev environment and especially outside of windows. Let me know if you get stuck and i will try to help!

Advertisement
Well im probably gonna need your help after all because although it works great, i found a problem. If one client disconnects and reconnects, they are unable to. And i found out why. One client locked in accept(), accept() is trying to accept that client at that temp id, even if the temp id changes from the other thread. So if client[2] is locked in accept, and the temp id changes to 1 because client 1 disconnects, thus making 1 the next client, it accepts client 2 while trying to work with client 1, thus making client 1 an invalid socket. This sucks cause there seems to be no way of changing the client array while its stuck in accept()

For a proper production system a async socket system is needed. This usually means async dns too


I'm not sure I agree.

First, if you use UDP, you only ever need one socket, period. You don't need async, and you don't even need select().

Second, even for TCP, modern servers can do fine with hundreds of sockets in select(). Most games don't have more than hundreds of clients connected to the same process, because even if there are more players in the world, the world is split in many processes.

Third, I don't see why a game server would need to use DNS at all. DNS for servers is only really useful if they do call-outs to third-party services, or for logging, when turning a numeric IP into a textual name. Many games don't use third party services at all, and in logging, it's generally better to log the raw (numeric) data, and turn it to text only if and when needed when viewing.

So if client[2] is locked in accept, and the temp id changes to 1 because client 1 disconnects


Clients don't call accept(). Only the server calls accept().

The server doesn't know which client is connecting, until accept() returns, and returns a particular IP address. And, evenso, it can't know the specific client from that IP address until the protocol supplies some kind of user-and-game token, or name/password, for authentication. Multiple processes share a single IP address, and (especially with NAT firewalls,) multiple game clients can share the same remote IP address (but not IP address AND port -- that's where source ports come into the protocol design equation!)
enum Bool { True, False, FileNotFound };

So if client[2] is locked in accept, and the temp id changes to 1 because client 1 disconnects


Clients don't call accept(). Only the server calls accept().

The server doesn't know which client is connecting, until accept() returns, and returns a particular IP address. And, evenso, it can't know the specific client from that IP address until the protocol supplies some kind of user-and-game token, or name/password, for authentication. Multiple processes share a single IP address, and (especially with NAT firewalls,) multiple game clients can share the same remote IP address (but not IP address AND port -- that's where source ports come into the protocol design equation!)

I never said clients call accept. Obviously its the server end. I said as an example, client[2] is LOCKED in the accept() loop. And if you attempt to change the index of the array through another thread, and a client connects to the server, it still accepts client[2] rather than whatever index you are wanting.

Its like this:


 
int main()
{
            temp_id = 2;
            while(1)
            {
                      cout << "Client id before accepting): " << temp_id << endl;
                      client[temp_id].socket = accept(server_socket, NULL, NULL);
                      cout << "Client id after accepting): " << temp_id << endl;
                      some_thread = thread(my_function, temp_id);
            }
            return 0;
}
 
int my_function(int &new_id)
{
      while(1)
      {
          
           new_id = 1;
      }
      return 0;
}

I know the code is sloppy and wont work as is but bare with me. The point Im trying to make is that if you are trying to change the index of the array while the client variable is stuck on accept until another an actual client connects to the server, its going to work with that index rather than whatever index you are changing from another thread. And I was wondering if there is a way around it such as possibly canceling out of the accept(). Or maybe there is a way to change the index while its locked on accept(). Otherwise I might as well say that multithreading a simple chat program isn't possible and resort to asyncronous or select.

You might use dns within a server application for many reasons. When i wrote a tcp based server which had to scale to tens of thousands of concurrent connections dns being async was a must. One udp socket was used, and requests queued. The dns requests were used for many things such as dns blacklist lookups, part of the authentication mechanisms and for outbound connections to allow servers to link together in a spanning tree.

That server abstracted the socket multiplexing and was mostly single threaded, using select or poll on windows, epoll on Linux, kqueue on freebsd and variants and iocp on solaris.

This is probably above and beyond what most game lobby servers etc might handle and the use case is vastly different but still a lot of the experience is transferable to other things like writing game servers...

Advertisement

I never said clients call accept. Obviously its the server end. I said as an example, client[2] is LOCKED in the accept() loop. And if you attempt to change the index of the array through another thread, and a client connects to the server, it still accepts client[2] rather than whatever index you are wanting.

This is called a race condition. This is NOT considered thread safe programming.

The only thread that should be resizing or messing with your socket array should be the one calling accept().

What you may want to do is only make changes to the array in the accept() thread.

You will need to provide a pointer to the socket in the array (like &sockets[x] ) so that the client thread can assign a "kill me I'm disconnected" value (invalid socket) to it in the array without knowledge of the array.

The thread that calls accept() will reuse these invalid socket spots by putting a valid socket in there and creating a new thread like before.

However, this approach is flawed because.....

it looks like you are going to attempt to send data from one thread and recv data from another thread on the same socket without using some sort of synchronization.

Using a mutex is really not optional in this situation.

The ideal case is to actually just pass the data to the other threads that own the client sockets (have like a linked list of your message objects protected by a mutex).

Other threads can add messages by calling a method from your server_client class and passing data that is shoved into the list. The thread that owns the socket will call a method that removes the oldest message from this list and then you send() it.


Otherwise I might as well say that multithreading a simple chat program isn't possible and resort to asyncronous or select.

it's entirely possible. The problem is you are making the assumption that you can just throw single-threaded mindset coding into a multithreaded project and have it work.

Writing code for multithreading is really a big task in itself. Multithreading itself is more complicated than networking.

LOL. I'm adding this edit because my original reply didn't make sense when I came back and read it....

Try this:

Encapsulate your server's sessions (a session is defined as a server-to-client relationship) using a server_client class (not using C++ but just C? use a struct to store it- C can be fairly object-ty if you want it to be).

Each server_client represents 1 thread, 1 socket (, and 1 id in this example i guess).

Each server_client has an outgoing message queue. Other server_clients add to this queue and never touch the socket directly.

Each server_client will check this queue for new messages and send() them out. (USE A MUTEX to prevent more than one thread from resizing this queue at the same time)

Somewhere there needs to be a list of server_client's (also protected by a mutex!) and every server_client will need to have access to this list for "broadcasting" messages out.

When the connection is closed, the server_client has code you call ( server_client->isClosed() ). The thread that accepts connections and spins up server_client's will have to be the one that deletes closed server_client's from the list.

For most apps, even highly scalable servers, multithreaded i/o isn't something that is needed and will add to complexity of the program.

As i had said previously it is possible to write a single threaded app to work with multiple sockets easily. I would recommend staying away from select() for any server that has lots of connections though, as you must iterate the entire FD_SET each time around your event loop, most oses have much better ways of dealing with this that arent O(n).

I know you are determined to get your simple threaded solution working but it is a dead end - take it from us and start again.

client[2] is LOCKED in the accept() loop. And if you attempt to change the index of the array through another thread, and a client connects to the server, it still accepts client[2] rather than whatever index you are wanting.


I see -- "client" as in the specific variable in your program.

I don't see how the temp_id variable gets modified at all in the code you posted. It will always have the value 2. Unless the "thread" function takes temp_id by reference. However, because temp_id seems to be globally declared, something outside the loop could affect it. Mutating global state is generally a terrible idea, and is almost never the right choice for threaded code.

The way I would write a similar function would be something like:


void accept_loop() {
    while (true) {
        SOCKET incoming = accept(listensocket, NULL, NULL);
        if (incoming < 0) break;
        lock l(client_list_lock);
        for (auto &ptr : client_list) {
            if (ptr.socket == INVALID_SOCKET) {
                ptr.socket = incoming;
                ptr.thread = thread(ptr); // if you really have to
                goto done;
            }
        }
        std::cerr << "Could not find a slot for incoming client." << std::endl;
        closesocket(incoming);
done:
        ;
    }
}
Although I'd probably use find_if, or more likely just keep a dynamic hash table of clients and create a new record for it.

Also, thread-per-client is an anti-pattern in multi-user games programming. You either want single-thread (and scale on a machine by using multiple processes) or thread-per-subsystem (physics, networking, AI, etc,) or thread-per-CPU (with a work item queue.)

For the single-threaded case, you can just use select() for all the sockets -- including the listening socket.
enum Bool { True, False, FileNotFound };

My god you are right. Instead of using an array, use a temperary variable such as incoming. Then I should loop through the list to find the next INVALID_SOCKET and make that a valid client. It works! My clients can disconnect and reconnect and it becomes the right client number. Heres the new code:

#include <iostream>
#include <winsock2.h>
#include <ws2tcpip.h>
#include <string>
#include <thread>
#include <vector>

#pragma comment (lib, "Ws2_32.lib")

#define IP_ADDRESS "192.168.56.1"
#define DEFAULT_PORT "3504"
#define DEFAULT_BUFLEN 512

struct client_type
{
    int id;
    SOCKET socket;
};

const char OPTION_VALUE = 1;
const int MAX_CLIENTS = 5;

//Function Prototypes
int process_client(client_type &new_client, std::vector<client_type> &client_array, int &num_clients, int &new_id, std::thread &thread);
int main();

int process_client(client_type &new_client, std::vector<client_type> &client_array, int &num_clients, int &new_id, std::thread &thread)
{
    std::string msg = "";
    char tempmsg[DEFAULT_BUFLEN] = "";

    //Session

    std::cout << "Client #" << new_client.id << " Accepted" << std::endl;
    num_clients++;

    while (1)
    {
        memset(tempmsg, 0, DEFAULT_BUFLEN);

        if (new_client.socket != 0)
        {
            int iResult = recv(new_client.socket, tempmsg, DEFAULT_BUFLEN, 0);

            if (iResult > 0)
            {
                if (strcmp("", tempmsg))
                    msg = "Client #" + std::to_string(new_client.id) + ": " + tempmsg;

                std::cout << msg.c_str() << std::endl;

                //Broadcast that message to the other clients
                for (int i = 0; i < MAX_CLIENTS; i++)
                {
                    if (client_array[i].socket != INVALID_SOCKET)
                        iResult = send(client_array[i].socket, msg.c_str(), strlen(msg.c_str()), 0);
                }
            }
            else
            {
                msg = "Client #" + std::to_string(new_client.id) + " Disconnected";

                std::cout << msg << std::endl;

                closesocket(new_client.socket);
                closesocket(client_array[new_client.id].socket);
                client_array[new_client.id].socket = INVALID_SOCKET;
                num_clients--;
                if (num_clients <= 0)
                    num_clients = 0;

                //Broadcast that message to the other clients
                for (int i = 0; i < MAX_CLIENTS; i++)
                {
                    if (client_array[i].socket != INVALID_SOCKET)
                        iResult = send(client_array[i].socket, msg.c_str(), strlen(msg.c_str()), 0);
                }


                //Create a temporary id for the next client
                new_id = -1;
                for (int i = 0; i < MAX_CLIENTS; i++)
                {
                    if (client_array[i].socket == INVALID_SOCKET)
                    {
                        client_array[i].id = i;
                        new_id = i;
                        break;
                    }
                }
                break;
            }
        }
    } //end while

    thread.detach();

    return 0;
}

int main()
{
    WSADATA wsaData;
    struct addrinfo hints;
    struct addrinfo *server = NULL;
    SOCKET server_socket = INVALID_SOCKET;
    std::string msg = "";
    std::vector<client_type> client(MAX_CLIENTS);
    int num_clients = 0;
    int temp_id = -1;
    std::thread my_thread[MAX_CLIENTS];

    //Initialize Winsock
    std::cout << "Intializing Winsock..." << std::endl;
    WSAStartup(MAKEWORD(2, 2), &wsaData);

    //Setup hints
    ZeroMemory(&hints, sizeof(hints));
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;
    hints.ai_flags = AI_PASSIVE;

    //Setup Server
    std::cout << "Setting up server..." << std::endl;
    getaddrinfo(static_cast<LPCTSTR>(IP_ADDRESS), DEFAULT_PORT, &hints, &server);

    //Create a listening socket for connecting to server
    std::cout << "Creating server socket..." << std::endl;
    server_socket = socket(server->ai_family, server->ai_socktype, server->ai_protocol);

    //Setup socket options
    setsockopt(server_socket, SOL_SOCKET, SO_REUSEADDR, &OPTION_VALUE, sizeof(int)); //Make it possible to re-bind to a port that was used within the last 2 minutes
    setsockopt(server_socket, IPPROTO_TCP, TCP_NODELAY, &OPTION_VALUE, sizeof(int)); //Used for interactive programs

    //Assign an address to the server socket.
    std::cout << "Binding socket..." << std::endl;
    bind(server_socket, server->ai_addr, (int)server->ai_addrlen);

    //Listen for incoming connections.
    std::cout << "Listening..." << std::endl;
    listen(server_socket, SOMAXCONN);

    //Initialize the client list
    for (int i = 0; i < MAX_CLIENTS; i++)
    {
        client[i] = { -1, INVALID_SOCKET };
    }

    while (1)
    {
        //If the number of clients is less than the maximum number of clients
        if (num_clients < MAX_CLIENTS)
        {

            SOCKET incoming = INVALID_SOCKET;
            incoming = accept(server_socket, NULL, NULL);

            if (incoming == INVALID_SOCKET) continue;

            //Create a temporary id for the next client
            for (int i = 0; i < MAX_CLIENTS; i++)
            {
                if (client[i].socket == INVALID_SOCKET)
                {
                    client[i].id = i;
                    temp_id = i;
                    break;
                }
            }

            client[temp_id].socket = incoming;

            //Broadcast the id to that client
            if (client[temp_id].socket != INVALID_SOCKET)
            {
                msg = std::to_string(client[temp_id].id);
                send(client[temp_id].socket, msg.c_str(), strlen(msg.c_str()), 0);
            }

            //Create a thread process for that client
            my_thread[temp_id] = std::thread(process_client, std::ref(client[temp_id]), std::ref(client), std::ref(num_clients), std::ref(temp_id), std::ref(my_thread[temp_id]));
        }
        else
        {
            std::cout << "Server is full" << std::endl;
            //So in order to prevent a crash (ie. -1 in an array)
            //we create a dummy client to accept the clients that
            //exceeded what we want in our server.
            SOCKET temp_client;
            temp_client = accept(server_socket, NULL, NULL);
            closesocket(temp_client);
        }
    } //end while


    //Close listening socket
    closesocket(server_socket);

    //Close client socket
    for (int i = 0; i < MAX_CLIENTS; i++)
    {
        my_thread[i].detach();
        closesocket(client[i].socket);
    }

    //Clean up Winsock
    WSACleanup();
    std::cout << "Program has ended successfully" << std::endl;

    system("pause");
    return 0;
}

This topic is closed to new replies.

Advertisement