Unfortunately, that code is not thread safe.

Also, please DON'T share the socket between threads in a way where you cannot insure that no two or more threads will try to send() and recv() at the same time.

I don't think berkely sockets or winsock is thread safe for that scenario.

Anytime you take something from a library you didn't write yourself, treat it as not thread safe and wrap its use with a mutex.

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.)

It would also become slower performance wise to have a thread per client due to all of the expensive context switches.

Unless of course you actually had a server with 1 CPU core per every client you ever expected to have :p.

thread-per-CPU (with a work item queue.)

I've been fooling around with this one. It's a steep learning curve but once you figure out how to structure it in C++ it becomes really cool.

I'm home from work. I was curious on everyone's opinion on my new code. This new code should be thread safe, and it works great. Like I said in the last post, I removed some variables from the thread function, counted my clients differently, and when the server is full, any new clients that log in get sent the message that the server is full. It runs flawlessly thus far.

Server:

#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, std::thread &thread);
int main();

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

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

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

            if (iResult != SOCKET_ERROR)
            {
                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)
                        if (new_client.id != i)
                            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;

                //Broadcast the disconnection 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);
                }

                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)
    {

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

        if (incoming == INVALID_SOCKET) continue;

        //Reset the number of clients
        num_clients = -1;

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

            if (client[i].socket != INVALID_SOCKET)
                num_clients++;

            //std::cout << client[i].socket << std::endl;
        }

        if (temp_id != -1)
        {
            //Send the id to that client
            std::cout << "Client #" << client[temp_id].id << " Accepted" << std::endl;
            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(my_thread[temp_id]));
        }
        else
        {
            msg = "Server is full";
            send(incoming, msg.c_str(), strlen(msg.c_str()), 0);
            std::cout << msg << std::endl;
        }
    } //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;
}

Client:

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

using namespace std;

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

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

struct client_type
{
    SOCKET socket;
    int id;
    char received_message[DEFAULT_BUFLEN];
};

int process_client(client_type &new_client);
int main();

int process_client(client_type &new_client)
{
    while (1)
    {
        memset(new_client.received_message, 0, DEFAULT_BUFLEN);

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

            if (iResult != SOCKET_ERROR)
                cout << new_client.received_message << endl;
            else
            {
                cout << "recv() failed: " << WSAGetLastError() << endl;
                break;
            }
        }
    }

    if (WSAGetLastError() == WSAECONNRESET)
        cout << "The server has disconnected" << endl;

    return 0;
}

int main()
{
    WSAData wsa_data;
    struct addrinfo *result = NULL, *ptr = NULL, hints;
    string sent_message = "";
    client_type client = { INVALID_SOCKET, -1, "" };
    int iResult = 0;
    string message;

    cout << "Starting Client...\n";

    // Initialize Winsock
    iResult = WSAStartup(MAKEWORD(2, 2), &wsa_data);
    if (iResult != 0) {
        cout << "WSAStartup() failed with error: " << iResult << endl;
        return 1;
    }

    ZeroMemory(&hints, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    cout << "Connecting...\n";

    // Resolve the server address and port
    iResult = getaddrinfo(static_cast<LPCTSTR>(IP_ADDRESS), DEFAULT_PORT, &hints, &result);
    if (iResult != 0) {
        cout << "getaddrinfo() failed with error: " << iResult << endl;
        WSACleanup();
        system("pause");
        return 1;
    }

    // Attempt to connect to an address until one succeeds
    for (ptr = result; ptr != NULL; ptr = ptr->ai_next) {

        // Create a SOCKET for connecting to server
        client.socket = socket(ptr->ai_family, ptr->ai_socktype,
            ptr->ai_protocol);
        if (client.socket == INVALID_SOCKET) {
            cout << "socket() failed with error: " << WSAGetLastError() << endl;
            WSACleanup();
            system("pause");
            return 1;
        }

        // Connect to server.
        iResult = connect(client.socket, ptr->ai_addr, (int)ptr->ai_addrlen);
        if (iResult == SOCKET_ERROR) {
            closesocket(client.socket);
            client.socket = INVALID_SOCKET;
            continue;
        }
        break;
    }

    freeaddrinfo(result);

    if (client.socket == INVALID_SOCKET) {
        cout << "Unable to connect to server!" << endl;
        WSACleanup();
        system("pause");
        return 1;
    }

    cout << "Successfully Connected" << endl;

    //Obtain id from server for this client;
    recv(client.socket, client.received_message, DEFAULT_BUFLEN, 0);
    message = client.received_message;

    if (message != "Server is full")
    {
        client.id = atoi(client.received_message);

        thread my_thread(process_client, client);

        while (1)
        {
            getline(cin, sent_message);
            iResult = send(client.socket, sent_message.c_str(), strlen(sent_message.c_str()), 0);

            if (iResult <= 0)
            {
                cout << "send() failed: " << WSAGetLastError() << endl;
                break;
            }
        }

        //Shutdown the connection since no more data will be sent
        my_thread.detach();
    }
    else
        cout << client.received_message << endl;

    cout << "Shutting down socket..." << endl;
    iResult = shutdown(client.socket, SD_SEND);
    if (iResult == SOCKET_ERROR) {
        cout << "shutdown() failed with error: " << WSAGetLastError() << endl;
        closesocket(client.socket);
        WSACleanup();
        system("pause");
        return 1;
    }

    closesocket(client.socket);
    WSACleanup();
    system("pause");
    return 0;
}

There is still the possibility that your code will attempt to send data on an invalid socket.

You see, each thread could possibly be running on a different CPU with each having their own cached view of parts of RAM.

The the cache on one CPU says that the socket is invalid, but the cache on another hasn't been updated yet...you see where this is going.

You use what are called "atomic" operations for situations like this.

Although i think it would just silently throw an error and continue working in this case.

Thanks for the feedback. But so far there has been no problems at all. So I'm gonna walk you guys through the code just to prove my point.

In the server, lets disregard the initialization process, as thats a given. In the actual loop in main(), I created a temporary variable SOCKETS incoming to be used to accept the next client.

    while (1)
    {

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

That is where it gets caught and wont continue unless a client logs onto my server. When a client does, it checks if its an INVALID_SOCKET still. If it is, it skips all the code in the loop and starts the loop over to try accepting another client. When and if it succeeds, it can be put into my client array with an id and a socket by finding the next empty client. The for loop will also count the number of valid client sockets, giving me the proper number of clients.

        if (incoming == INVALID_SOCKET) continue;

        //Reset the number of clients
        num_clients = -1;

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

            if (client[i].socket != INVALID_SOCKET)
                num_clients++;

            //std::cout << client[i].socket << std::endl;
        }

If the temp id is not -1, it can proceed in sending that id given to that client, which only happens once the client logs on. It can then run the other thread. If the temp id is still -1, then the server is full. On top of that, itll send the message using the "incoming" socket (my temp socket I used earlier) to that client that attempted to log on with the message "server is full".

        if (temp_id != -1)
        {
            //Send the id to that client
            std::cout << "Client #" << client[temp_id].id << " Accepted" << std::endl;
            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(my_thread[temp_id]));
        }
        else
        {
            msg = "Server is full";
            send(incoming, msg.c_str(), strlen(msg.c_str()), 0);
            std::cout << msg << std::endl;
        }
    } //end while

My loop will cycle again, and once again get caught on accept. That is thread 1. It loops once and gets caught on accept() every time, and will only do so when a client logs on.

Now my other thread (which is actually an array of threads), is created for every new client. And those loop only once before getting caught on recv(). So lets say I have all 5 clients logged on:

-0---1-----2----3----4

I have 5 different threads each receiving messages from those clients. It never has once gotten confused which socket to receive from because I'm using an array of threads, each corresponding to a particular client, and never a mishmash of data due do race conditions as you all propose would happen. Because each index of the thread array is working with each index of my client array. 0 is working with 0, 1 is working with 1, 2 is working with 2, etc. It is impossible for thread 0 to receive data from client 2, or thread 3 to receive data from client 1, or whatever the case may be, it has not happened nor will it ever happen. It is not continuous loop that goes for whatever frames per second, but rather a loop that loops once before getting caught, just like the loop in my main(). It will only cycle if it receives a message, all 5 of my threads. If a client disconnects, that thread will detach. And can be recreated if that particular client logs back on.

So with main() accepting clients on one thread while at the same time 5 other threads are receiving messages from public clients, I still cant see where a race condition would occur. Because my loops only begin to cycle when something happens, similar to an event. The only thing that could ever happen is during the console window, text from one client could potentially continue on from another if they sent their messages at the same time. This is unavoidable in a console window, but controllable in a windows GUI or even a DirectX game environment. And would probably look something like this:

2 messages being displayed simultaniously

Hey hoHEY WHATS UP!w are you?

Unless of course I can capture the messages and shove them into a string array list of messages and display them appropriately. That is so far the only problem with it, but other than that, has worked flawlessly, mostly thanks to you guys for leading me in the right direction. I know I could use Select(), Event based, or even Asyncrounous sockets, but I wanted to see how possible it would be to attempt a multithread version of a simple chat program.