Multithreaded Client/ Server communication

Question

This is my first network programming codes writing for a client who has the following requirement:

1. My Server has to run 24*7*365 for multiple clients at the same time (concurrency).
2. Their Client (Software running on client machine) has some bugs which makes it to crash sometimes.
3. When the software crashes, Server should get information and the connection should be terminated, freeing any other id's associated with each connected client.

As of now, they are not able to identify whether client side software has really crashed or is still running. The first time client gets registered in the server succesfully, they are not able to track about its active execution state on later stages. So I gave it a thought and somehow managed to use threads to satisfy their requirement partially (code still under development). But is this really a fool-proof solution? Will this code be able to handle 100's of clients and know about their statuses? I tried with around 10-20 terminals of client and it worked perfectly.

Can anyone review my code let me know the errors/ any modifications.

Here's my Server code:

#include <stdio.h>
#include <string.h>    //strlen
#include <stdlib.h>    //strlen
#include <sys/socket.h>
#include <arpa/inet.h> //inet_addr
#include <unistd.h>    //write
#include <pthread.h> //for threading , link with lpthread

//the thread function
void *connection_handler(void *);

int main(int argc , char *argv[])
{
    int socket_desc , client_sock , c , *new_sock;
    struct sockaddr_in server , client;

    //Create socket
    socket_desc = socket(AF_INET , SOCK_STREAM , 0);
    if (socket_desc == -1)
    {
        printf("Could not create socket");
    }
    puts("Socket created");

    //Prepare the sockaddr_in structure
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = INADDR_ANY;
    server.sin_port = htons( 8888 );

    //Bind
    if( bind(socket_desc,(struct sockaddr *)&server , sizeof(server)) < 0)
    {
        //print the error message
        perror("bind failed. Error");
        return 1;
    }
    puts("bind done");

    //Listen
    listen(socket_desc , 3);

    //Accept and incoming connection
    puts("Waiting for incoming connections...");
    c = sizeof(struct sockaddr_in);


    //Accept and incoming connection
    /*puts("Waiting for incoming connections...");
    c = sizeof(struct sockaddr_in);*/

    while( (client_sock = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)) )
    {
        puts("Connection accepted");

        pthread_t sniffer_thread;
        new_sock = malloc(1);
        *new_sock = client_sock;

        if( pthread_create( &sniffer_thread , NULL ,  connection_handler , (void*) new_sock) < 0)
        {
            perror("could not create thread");
            return 1;
        }

        //Now join the thread , so that we dont terminate before the thread
        //pthread_join( sniffer_thread , NULL); // was commented before
        puts("Handler assigned");
    }

    if (client_sock < 0)
    {
        perror("accept failed");
        return 1;
    }

    return 0;
}

/*
 * This will handle connection for each client
 * */
void *connection_handler(void *socket_desc)
{
    //Get the socket descriptor
    int sock = *(int*)socket_desc;
    int read_size;
    char *message , client_message[2000];

    //Receive a message from client
    while( (read_size = recv(sock , client_message , 2000 , 0)) > 0 )
    {
        //Send the message back to client
        write(sock , client_message , strlen(client_message));
        printf("%s\n",client_message);
    }

    if(read_size == 0)
    {
        puts("Client disconnected");
        fflush(stdout);
    }
    else if(read_size == -1)
    {
        perror("recv failed");
    }

    //Free the socket pointer
    free(socket_desc);
    close(sock);
    pthread_exit(NULL); 
    return;
}

Here's my client:

#include<stdio.h> //printf
#include<string.h>    //strlen
#include<sys/socket.h>    //socket
#include<arpa/inet.h> //inet_addr

int main(int argc , char *argv[])
{
    int sock;
    struct sockaddr_in server;
    char message[1000] , server_reply[2000];

    //Create socket
    sock = socket(AF_INET , SOCK_STREAM , 0);
    if (sock == -1)
    {
        printf("Could not create socket");
    }
    puts("Socket created");

    server.sin_addr.s_addr = inet_addr("127.0.0.1");
    server.sin_family = AF_INET;
    server.sin_port = htons( 8888 );

    //Connect to remote server
    if (connect(sock , (struct sockaddr *)&server , sizeof(server)) < 0)
    {
        perror("connect failed. Error");
        return 1;
    }

    puts("Connected\n");

    //keep communicating with server
    while(1)
    {
        printf( "Type 'q' to end the session:\n");
        printf("Now type your message:");
        scanf("%s" , message);

        if( message[0] == 'q')
        {
             close(sock);
             return 0;
        }

        else{ 

        //Send some data
        if( send(sock , message , strlen(message) , 0) < 0)
        {
            puts("Send failed");
            return 1;
        }

        //Receive a reply from the server
        if( recv(sock , server_reply , 2000 , 0) < 0)
        {
            puts("recv failed");
            break;
        }

        puts("Server reply : ");
        puts(server_reply);
        }
    }

    close(sock);
    return 0;
}

Answer 1

Should not assume null termination

This code in the server assumes that the client will send null terminated data:

//Receive a message from client
while( (read_size = recv(sock , client_message , 2000 , 0)) > 0 )
{
    //Send the message back to client
    write(sock , client_message , strlen(client_message));
    printf("%s\n",client_message);
}

There are several problems with that:

1. The client could not send null terminated data.
2. The client could send a string longer than 2000 bytes, which would look unterminated.
3. Even if the client were well behaved, sockets don't necessarily send/receive all of their data in one chunk. So the client might send a 1000 byte string but your recv() call might only read 500 bytes (with no null termination).

To fix this,you should use read_size to terminate the string yourself (and make your buffer bigger by one character). Or pick a new message protocol that sends a length followed by a string of that length, so you can better determine where each message ends.

Answer 2

• If the client sends some data and immediately disconnects, the server's write will fail, and the server receives a SIGPIPE signal. This signal is fatal, and server is terminated. You need to handle SIGPIPE.

• write is not guaranteed to send a complete message. You need to wrap it into a loop.

Answer 3

You have a bug here:

int socket_desc , client_sock , c , *new_sock;
new_sock = malloc(1);

You're allocating 1 byte with the malloc, int's are usually bigger than this (typically 4 bytes), so you're borrowing 3 bytes that you don't own. You should probably be doing:

new_sock = malloc(sizeof *new_sock);

I'm also not a huge fan of multiple variable declarations on a single line, it makes the code harder to read. This is amplified when you're mixing normal variables and pointers, which you are here:

int socket_desc , client_sock , c , *new_sock;

Your approach of a single thread per client is probably going to be ok while you have a reasonable number of clients, however it may have scale-ability issues if you have a large number of clients. Typically having a pool of threads to manage the connections can be more efficient.

Answer 4

A memset should come after printf so that it clears the buffer otherwise when the client sends new message the buffer is already filled and also note that i have added one in strlen when writing to client so it also sends the null terminator otherwise you will see garbage values at the end of the string in client

while( (read_size = recv(sock , client_message , 2000 , 0)) > 0 )
    {
        //Send the message back to client

        write(sock , client_message , strlen(client_message)+1);
        printf("%s\n",client_message);

        memset(client_message ,'\0', 2000);
    }