Sign in to follow this  

Switching from CreateThread() to Thread

This topic is 1102 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I made a simple chat program using a simple console window, but have one problem. Originally both my client and server were using CreateThread(), but saw some more benefits of using the thread header so I began switching it. I did the client first and so far everything works with no problems. But then I tried the server end, and the program went haywire in the loop. With no client open at all, simply running the server kept accepting clients and disconnecting clients over and over rapidly. Its suppose to pause at accept and bare in mind, no client was open at all!

After careful debugging and putting those couple of lines of code back the way it was back to CreateThread(), I found simply having #include <thread> uncommented was making my loop go haywire. I wasnt even doing anything with the header. Whats making my program act that way from that thread header? Thanks in advance

Share this post


Link to post
Share on other sites

Anyways sorry that I didnt post any code to show you what I mean. I posted that off my phone. Now that I'm home, here is what I'm talking about.

 

This server code goes haywire if you were to uncomment #include <thread>.  Otherwise it works great.

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

//NOTE: Uncommenting this makes the program
//              go haywire
//#include <thread>

using namespace std;

#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 maxClients = 5;

//Function Prototypes
DWORD WINAPI process_client(LPVOID Parameter);
int main();

WSADATA wsaData;

struct addrinfo hints;
struct addrinfo *server = NULL;

SOCKET server_socket = INVALID_SOCKET;
client_type client[maxClients];

int num_clients = 0;

string msg = "";
char tempmsg[DEFAULT_BUFLEN] = "";

int temp_id = -1;

DWORD WINAPI process_client(LPVOID Parameter)
{
    client_type local_socket;
    local_socket = *(static_cast<client_type *>(Parameter));

    //Session
    if (num_clients < maxClients)
    {
        cout << "Client #" << local_socket.id << " Accepted" << endl;
        num_clients++;
    
        while (1)
        {
            memset(tempmsg, 0, DEFAULT_BUFLEN);

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

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

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

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

                    cout << msg << endl;

                    closesocket(local_socket.socket);
                    closesocket(client[local_socket.id].socket);
                    client[local_socket.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 < maxClients; i++)
                    {
                        if (client[i].socket != INVALID_SOCKET)
                            iResult = send(client[i].socket, msg.c_str(), strlen(msg.c_str()), 0);
                    }


                    //Create a temporary id for the next client
                    temp_id = -1;
                    for (int i = 0; i < maxClients; i++)
                    {
                        if (client[i].socket == INVALID_SOCKET)
                        {
                            client[i].id = i;
                            temp_id = i;
                            break;
                        }
                    }
                    break;
                }
            }
        }
    }
    else
    {
        cout << "Server is full." << endl;
    }

    return 0;
}

int main()
{
    //Initialize Winsock
    cout << "Intializing Winsock..." << 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
    cout << "Setting up server..." << endl;
    getaddrinfo(static_cast<LPCTSTR>(IP_ADDRESS), DEFAULT_PORT, &hints, &server);

    //Create a listening socket for connecting to server
    cout << "Creating server socket..." << 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.
    cout << "Binding socket..." << endl;
    bind(server_socket, server->ai_addr, (int)server->ai_addrlen);

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

    for (int i = 0; i < maxClients; i++)
    {
        client[i] = {-1, INVALID_SOCKET };
    }

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

        client[temp_id].socket = accept(server_socket, NULL, NULL);

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

        DWORD threadID;
        CreateThread(NULL, 0, process_client, (LPVOID)&client[temp_id], 0, &threadID);
    }

    //Close listening socket
    closesocket(server_socket);

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

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

    system("pause");
    return 0;
}

I want to be able to switch from CreateThread to using Thread though since my client side uses Thread and maintain consistency. Any bright ideas? Thanks:

#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();

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

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 > 0)
                cout << new_client.received_message << endl;
            else if (iResult <= 0)
            {
                cout << "recv() failed: " << WSAGetLastError() << endl;
                break;
            }
        }

    }

    return 0;
}

int main()
{
    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 << endl;

    //Obtain id from server for this client;
    recv(client.socket, client.received_message, DEFAULT_BUFLEN, 0);
    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 << "sent() failed: " << WSAGetLastError() << endl;
            break;
        }
    }

    //Shutdown the connection since no more data will be sent
    my_thread.detach();
    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;
}
Edited by Psychopathetica

Share this post


Link to post
Share on other sites

At its current state, how is it not thread safe already if its working ok? Maybe I missed something perhaps?

 

That is why Im switching to Thread because I want to add more arguments and reduce any globals possible, and have control over it. But that #include statement really is messing it up for me and I still cant figure out why.

Edited by Psychopathetica

Share this post


Link to post
Share on other sites

Just because you application works doesn't means it thread-safe. That is the beauty or curse of the non-deterministic nature of multithreaded application. If the code is not thread-safe, then of course its going to break on simple changes. Switching threading library shouldn't cause a working application to go haywire provided there is one-to-one correspondence in the expression of the threading primitives. Writing threaded code without taking into consideration synchronization issues is tantamount to coding suicide. Like ApochPiQ said, the code is riddle with issues that are not thread-safe. I suggest you take a step back and reason at a higher level how each subsystem could be potentially accessed at the SAME time via multiple threads and what would/should happen in that case.

Share this post


Link to post
Share on other sites
I agree that using the standard library is a good idea. However, IMO, it's a little bit cart-before-horse to try to learn <thread> and company before learning the fundamentals of how threading works and how to write threaded code. There are theory requirements, and just diving in and hacking around is likely to develop into extremely bad habits. So I advocate for learning the theory first, and then learning application via <thread> et. al.

Share this post


Link to post
Share on other sites

Well I just did some more experimenting. I commented anything that had to do with threading at all, including the function (even the client end I removed all threading) and moved all my variables inside main(). Uncommenting #include <thread> makes the server unable to accept clients, even if I put a while loop around the accept() function which you see. But if I comment out the #include statement, the client connects just fine. So the <thread> library is fiddling with the winsock function for some reason.

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

//NOTE: Uncommenting this makes the program
//              go haywire
//#include <thread>

using namespace std;

#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;
};

//Function Prototypes
//DWORD WINAPI process_client(LPVOID Parameter);
//int process_client(client_type &new_client, int &num_clients, int &new_id);

int main();

//DWORD WINAPI process_client(LPVOID Parameter)
//int process_client(client_type &new_client, int &num_clients, int &new_id)
//{
    //Session
    //if (num_clients < maxClients)
    //{
        //cout << "Client #" << new_client.id << " Accepted" << endl;
        //num_clients++;

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

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

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

        //            cout << msg.c_str() << endl;

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

        //            cout << msg << endl;

        //            closesocket(local_socket.socket);
        //            closesocket(client[local_socket.id].socket);
        //            client[local_socket.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 < maxClients; i++)
        //            {
        //                if (client[i].socket != INVALID_SOCKET)
        //                    iResult = send(client[i].socket, msg.c_str(), strlen(msg.c_str()), 0);
        //            }


        //            //Create a temporary id for the next client
        //            temp_id = -1;
        //            for (int i = 0; i < maxClients; i++)
        //            {
        //                if (client[i].socket == INVALID_SOCKET)
        //                {
        //                    client[i].id = i;
        //                    temp_id = i;
        //                    break;
        //                }
        //            }
        //            break;
        //        }
        //    }
        //}
    //}
    //else
    //{
    //    cout << "Server is full." << endl;
    //}

    //return 0;
//}

int main()
{
    const char option_value = 1;
    const int maxClients = 5;

    WSADATA wsaData;

    struct addrinfo hints;
    struct addrinfo *server = NULL;

    SOCKET server_socket = INVALID_SOCKET;
    client_type client[maxClients];

    int num_clients = 0;

    string msg = "";
    char tempmsg[DEFAULT_BUFLEN] = "";

    int temp_id = -1;


    //Initialize Winsock
    cout << "Intializing Winsock..." << 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
    cout << "Setting up server..." << endl;
    getaddrinfo(static_cast<LPCTSTR>(IP_ADDRESS), DEFAULT_PORT, &hints, &server);

    //Create a listening socket for connecting to server
    cout << "Creating server socket..." << 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.
    cout << "Binding socket..." << endl;
    bind(server_socket, server->ai_addr, (int)server->ai_addrlen);

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

    for (int i = 0; i < maxClients; i++)
    {
        client[i] = {-1, INVALID_SOCKET };
    }

    //thread my_thread;

    while (1)
    {
        cout << "MAIN: Begining loop" << endl;

        //Create a temporary id for the next client
        temp_id = -1;
        for (int i = 0; i < maxClients; i++)
        {
            if (client[i].socket == INVALID_SOCKET)
            {
                client[i].id = i;
                temp_id = i;
                break;
            }
        }
        
        cout << client[temp_id].socket << ", " << INVALID_SOCKET << endl;

        while (client[temp_id].socket == INVALID_SOCKET)
        {
            client[temp_id].socket = accept(server_socket, NULL, NULL);
        }

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

        cout << client[temp_id].socket << ", " << INVALID_SOCKET << endl;

        //my_thread = thread(process_client, client[temp_id], num_clients, temp_id);


        cout << "TEST NUM_CLIENTS: " << num_clients << endl;
        cout << "TEST TEMP_ID: " << temp_id << endl;



    //    DWORD threadID;
    //    CreateThread(NULL, 0, process_client, (LPVOID)&client[temp_id], 0, &threadID);
    }

    //if ((int)(my_thread.native_handle()) != 0xCCCCCCCC)
        //my_thread.detach();

    //Close listening socket
    closesocket(server_socket);

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

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

    system("pause");
    return 0;
}
Edited by Psychopathetica

Share this post


Link to post
Share on other sites

Both WinSock and C++11 (more specifically the header which in your case is included by ) define a bind function. Either don't use using namespace at a global scope (preferred), or be specific and use the global version of bind that WinSock defines by using ::bind.
That's a really excellent catch. I wonder how this can even successfully compile at all, though.

 

A Windows socket handle is simply a void*, so std::bind would create a function object that allows calling void*::operator() or something with two implicit parameters. I would expect the compiler to vigorously complain about that, and  I am also not sure what return type the compiler would invent for calling void* as a function. Do you even understand (or can you imagine) what calling void* as function could supposedly mean? I sure don't have an idea biggrin.png

Share this post


Link to post
Share on other sites
The call operator of the created functor is a template itself (perfect forwarding!), so that it isn't actually callable doesn't matter as that template isn't instanced as the return value is never used/called.

It would sure be great if we had gotten concepts in C++14, but we didn't.

Share this post


Link to post
Share on other sites

Thank you guys for all the help. I made it thread safe. Not a single global variable is used besides constants. Here is my new code if anyone wants to play around with it.

 

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 maxClients = 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 < maxClients; 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 < maxClients; 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 < maxClients; 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(maxClients);
    int num_clients = 0;
    int temp_id = -1;
    std::thread my_thread[maxClients];

    //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 < maxClients; i++)
    {
        client[i] = { -1, INVALID_SOCKET };
    }

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

        //If there are no more invalid sockets, the server is full
        if (temp_id != -1)
            client[temp_id].socket = accept(server_socket, NULL, NULL);
        else
        {
            //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);
        }

        //If the number of clients is less than the maximum number of clients
        if (num_clients < maxClients)
        {
            //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;
        }
    } //end while


    //Close listening socket
    closesocket(server_socket);

    //Close client socket
    for (int i = 0; i < maxClients; 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 > 0)
                cout << new_client.received_message << endl;
            else if (iResult <= 0)
            {
                cout << "recv() failed: " << WSAGetLastError() << endl;
                break;
            }
        }
    }

    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;

    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 << endl;

    //Obtain id from server for this client;
    recv(client.socket, client.received_message, DEFAULT_BUFLEN, 0);
    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 << "sent() failed: " << WSAGetLastError() << endl;
            break;
        }
    }

    //Shutdown the connection since no more data will be sent
    my_thread.detach();
    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;
}
Edited by Psychopathetica

Share this post


Link to post
Share on other sites

Globals are always a problem, but the real problem is accessing shared mutable state without proper synchronisation. You have many threads effectively touching the same data due to passing references / pointers to the thread functions, so unfortunately that code is not thread safe.

 

Threading is not one of those things you can just "pick up" by doing, you'll have to take some time to study from reliable sources.

Share this post


Link to post
Share on other sites

Threading is a subtle beast, even with experience I prefer a code review to ensure I've not made any mistakes. You're welcome to post your current implementation here to get confidence in its correctness.

Share this post


Link to post
Share on other sites
Getting rid of the globals is fine. But if you are just passing a reference to the same local to several threads, you are in basically the same position - unsynchronised access to the same data.

If I'm reading your revised code wrong, apologies.

Share this post


Link to post
Share on other sites

This topic is 1102 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this