Multithreaded client socket

Question

This is my first time working with networks and multithreading in C, and would like to know how to make improvements.

I wanted to make a client program that handles reading and writing to the server in two distinct threads to improve performance. I also wanted to have the user wait 3 seconds before allowing the user to send more information to the server. I am unsure if the way I ended the threads was acceptable, I used a global variable, or if I should use semaphores.

#include "client.h"

/*Static Variables*/
pthread_t threads[2];
int end = 0;
char command_buffer[256];                                                   // char array to store data  going to the server
char response_buffer[256];                                                      // char array to store data  coming from the server


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

    // Declare initial vars
   int sockfd = -1;                                                             // file descriptor for our socket
   int portno = -1;                                                             // server port to connect to
   struct sockaddr_in serverAddressInfo;                        // Super-special secret C struct that holds address info for building our socket
   struct hostent *serverIPAddress;                                 // Super-special secret C struct that holds info about a machine's address
   int iSetOption = 1;
   int err = -1;


    // If the user didn't enter enough arguments, complain and exit
   if (argc < 3)
   {
      fprintf(stderr,"usage %s hostname port\n", argv[0]);
      exit(0);
   }


    /** If the user gave enough arguments, try to use them to get a port number and address **/
    // convert the text representation of the port number given by the user to an int
    portno = atoi(argv[2]);

    // look up the IP address that matches up with the name given - the name given might
    //    BE an IP address, which is fine, and store it in the 'serverIPAddress' struct

   serverIPAddress = gethostbyname(argv[1]);
   if (serverIPAddress == NULL)
   {
        fprintf(stderr,"ERROR, no such host\n");
        exit(0);
   }

    // try to build a socket .. if it doesn't work, complain and exit
   sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
   if (sockfd < 0) 
   {
      error("ERROR creating socket");
   }


   err = setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, (char*)&iSetOption, sizeof(iSetOption));
   if (sockfd < 0) 
   {
      error("ERROR creating socket");
   }

    /** We now have the IP address and port to connect to on the server, we have to get    **/
    /**   that information into C's special address struct for connecting sockets                     **/

    // zero out the socket address info struct .. always initialize!
   bzero((char *) &serverAddressInfo, sizeof(serverAddressInfo));

    // set a flag to indicate the type of network address we'll be using 
   serverAddressInfo.sin_family = AF_INET;

    // set the remote port .. translate from a 'normal' int to a super-special 'network-port-int'
   serverAddressInfo.sin_port = htons(portno);

    // do a raw copy of the bytes that represent the server's IP address in 
    //   the 'serverIPAddress' struct into our serverIPAddressInfo struct
   bcopy((char *)serverIPAddress->h_addr, (char *)&serverAddressInfo.sin_addr.s_addr, serverIPAddress->h_length);




   while(connect(sockfd,(struct sockaddr *)&serverAddressInfo,sizeof(serverAddressInfo)) != 0) 
   {
    error("[-] ERROR connecting");
   }

    /** If we're here, we're connected to the server .. w00t!  Time to multithread**/

    err = pthread_create(&(threads[0]), NULL, &writeToServer, (void *) sockfd);

    if (err != 0)
    {
        error("\n[-]can't create thread :[%s]"); 
    }

    err = pthread_create(&(threads[1]), NULL, &readFromServer, (void *) sockfd);

    if (err != 0)
    {
        error("\n[-]can't create thread :[%s]"); 
    }


    pthread_join(threads[0], NULL);
    pthread_join(threads[1], NULL); 

    return 0;
}

void * writeToServer(void * args)
{
    int sockfd = *((int *) args);
    int n = -1;
    while(end != 1)
    {
        printf("[-] Please enter message");
        // zero out the message buffer
        bzero(command_buffer,256);

        // get a message from the client
        fgets(command_buffer,255,stdin);

        if(strcmp(command_buffer,"exit"))
        {
            end = 1;
        }

        // try to write it out to the server
        n = write(sockfd,command_buffer,strlen(command_buffer));

        if (n < 0)
        {
           error("ERROR writing to socket");
        }

        sleep(3);
    }
    return 0;
}

void * readFromServer(void * args)
{
    int sockfd = *((int *) args);
    int n = -1;
    while(end != 1)
    {
        bzero(response_buffer,256);

        // read a message from the server into the buffer
        n = read(sockfd,response_buffer,255);

        // if we couldn't read from the server for some reason, complain and exit
        if (n < 0)
        {
           error("ERROR reading from socket");
        }

        // print out server's message
        printf("%s\n",response_buffer);
    }

    return 0;
}


void error(char *msg)
{
   perror(msg);
   exit(0);
}

Answer 1

The posted code uses a 'home grown' header file, client.h. Please post the contents of that file .

In the function error(), the call exit(0); means the code was successful. What should be used is: exit( EXIT_FAILURE );.

The calls to read() and write() are acceptable for sockets to a local server. For remote servers, the posted code should use: send()andrecv()`.

The last parameter to pthread_create() should be a pointer, so this line:

err = pthread_create(&(threads[1]), NULL, &readFromServer, (void *) sockfd);

should be:

err = pthread_create(&(threads[1]), NULL, &readFromServer, (void *) &sockfd);

Notice the & before the sockfd parameter. Similar considerations pertain to the other call to pthread_create().

The operator return from the thread functions should be returning NULL rather than 0. The implicit conversion will correct that coding problem, but depending on the compiler to fix coding problems is a poor programming practice.

The read() function does NOT append a NUL byte to the input buffer, So lines like:

n = read(sockfd,response_buffer,255); 

should be:

n = read(sockfd,response_buffer,sizeof(response_buffer));  

since the function read() does not terminate the buffer with a NUL char, after checking for a successful call to read(), insert the statement:

response_buffer[n] = '\0';

In general, this is much better than calling bzero() before calling the read() function.

The posted code contains some 'magic' numbers. 'magic' numbers make the code much more difficult to understand, debug, maintain. I suggest using an enum statement or #definestatements to give those 'magic' numbers meaningful names, then using those meaningful names throughout the code. (one of the 'magic' numbers is 256)

The returned value from fgets() should be checked to assure it was successful. It is very easy for the user to type <ctrl><z> or similar, which would result in a 'empty' command_buffer[] and (in older versions of write() calling write() with a 0 length is usually a problem as the actual I/O decrements before checking for 'done' so a huge amount of data would be transmitted.

The posted code contains several 'abrupt' exit points. While the OS will 'usually' clean up when a program exits, That is a very poor practice for the program to not clean up after itself. In this case the program should (at each appropriate place) call

close(sockfd);  

The error() function fails to clean up by closing the socket (if open) and killing any threads (if any started).

For ease of readability and understanding by us humans: indent consistently. Indent after every opening brace '{'. Un-indent before every closing brace '}'.

There are several places in the posted code that are making implicit conversion including int to pointer, int to short, etc . Each of these can result in an invalid value being produced. I suggest compiling with all warnings enabled and fixing those warnings.