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 <unistd.h>    //write

void *connection_handler(void *);

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

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

server.sin_family = AF_INET;
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...");

//Accept and incoming connection
/*puts("Waiting for incoming connections...");

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

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

if( pthread_create( &sniffer_thread , NULL ,  connection_handler , (void*) new_sock) < 0)
{
return 1;
}

//Now join the thread , so that we dont terminate before the thread
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;
char *message , client_message[2000];

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

{
puts("Client disconnected");
fflush(stdout);
}
{
perror("recv failed");
}

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


Here's my client:

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

int main(int argc , char *argv[])
{
int sock;

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

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");
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;
}

if( recv(sock , server_reply , 2000 , 0) < 0)
{
puts("recv failed");
break;
}

}
}

close(sock);
return 0;
}

• Hi @Eichhörnchen thanks for the comment. The crash in the sense I closed the terminal when the connection has been accepted and in the server I can see the message: "Client disconnected". So I felt its working. Regarding heartbeat or KEEP_ALIVE, I am still searching resources for understanding the same. Share with me any good links if you have. – Sri Church Oct 5 '16 at 7:37

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

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.

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

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