Socket multithreading Implementation C

Question

I am working on a implementing a multithread multi client single server socket in C. However for whatever reason currently the program, when using pthread_create() to create a new thread, it does not advance past that line of code. I have put print lines before and after this line of code and all of the print lines before hand print fine but none of them after print. This leads me to believe that pthread_create() is somehow buggy. The strange thing about this is I can have 1 client connect and successfully sent/receive data from the server but because the loop that the listen() command is in is not advancing I cannot take on additional clients. I appreciate your help in this matter.

Server Code

#include <stdio.h>
#include <stdlib.h> //for IOs
#include <string.h>
#include <unistd.h>
#include <sys/types.h> //for system calls
#include <sys/socket.h> //for sockets
#include <netinet/in.h> //for internet
#include <pthread.h>
void error(const char *msg)
{
 perror(msg);
 exit(1);
}
void *threadFunc(int mySockFd)
{
 int n;
 char buffer[256];
 do
 {
 bzero(buffer,256);
 n = read(mySockFd,buffer,255);
 if (n < 0) 
 {
 error("ERROR reading from socket");
 }
 else if(strcmp(buffer, "EXIT\n") == 0)
 {
 printf("Exit by user\n");
 pthread_exit(NULL);
 }
 else
 {
 printf("Here is the message: %s\n",buffer);
 n = write(mySockFd,"I got your message",18); 
 if (n < 0) 
 {
 error("ERROR writing to socket");
 }
 }
 }while(n >= 0);
} 
int main(int argc, char *argv[])
{
 int sockfd;
 int newsockfd;
 int portno;
 pthread_t pth;
 int n; /*n is the return value for the read() and write() calls; i.e. it contains the number of characters read or written.*/
 int i = 0;
 printf("after var decl");
socklen_t clilen; /*clilen stores the size of the address of the client. This is needed for the accept system call.*/
 char buffer[256]; /*The server reads characters from the socket connection into this buffer.*/
 struct sockaddr_in serv_addr;
 struct sockaddr_in cli_addr;
if (argc < 2) 
 {
 fprintf(stderr,"ERROR, no port provided\n");
 exit(1);
 }
sockfd = socket(AF_INET, SOCK_STREAM, 0);
if (sockfd < 0)
 {
 error("ERROR opening socket");
 }
bzero((char *) &serv_addr, sizeof(serv_addr));
portno = atoi(argv[1]);
serv_addr.sin_family = AF_INET;
serv_addr.sin_addr.s_addr = INADDR_ANY;
serv_addr.sin_port = htons(portno);
if (bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0)
 {
 error("ERROR on binding");
 }
do
 { 
 printf("before listen");
 listen(sockfd,5);
 printf("after listen");
 clilen = sizeof(cli_addr);
 printf("before accept");
 newsockfd = accept(sockfd,(struct sockaddr *) &cli_addr,&clilen);
 printf("after accept");
 pthread_create(&pth,NULL,threadFunc(newsockfd),(void*) &i);
 printf("after pthread create");
 if (newsockfd < 0)
 {
 error("ERROR on accept");
 }
 }while(1 == 1);
bzero(buffer,256);
 n = read(newsockfd,buffer,255);
 if (n < 0) 
 {
 error("ERROR reading from socket");
 }
 printf("Here is the message: %s\n",buffer);
if (n < 0) error("ERROR writing to socket");
close(newsockfd);
 close(sockfd);
 return 0;

and here is the Client Code

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h> /*The file netdb.h defines the structure hostent, which will be used below.*/
void error(const char *msg)
{
 perror(msg);
 exit(0);
}
int main(int argc, char *argv[])
{
int sockfd;
int portno;
int n;
struct sockaddr_in serv_addr;
struct hostent *server;
char buffer[256];
if (argc < 3) 
{
 fprintf(stderr,"usage %s hostname port\n", argv[0]);
 exit(0);
}
 portno = atoi(argv[2]);
 sockfd = socket(AF_INET, SOCK_STREAM, 0);
if (sockfd < 0)
{
 error("ERROR opening socket"); 
}
server = gethostbyname(argv[1]);
if (server == NULL) 
{
 fprintf(stderr,"ERROR, no such host\n");
 exit(0);
}
bzero((char *) &serv_addr, sizeof(serv_addr));
serv_addr.sin_family = AF_INET;
bcopy((char *)server->h_addr,
(char *)&serv_addr.sin_addr.s_addr,
server->h_length);
serv_addr.sin_port = htons(portno);
if (connect(sockfd,(struct sockaddr *) &serv_addr,sizeof(serv_addr)) < 0)
{
 error("ERROR connecting");
}
do
{
 printf("Please enter the message: ");
 bzero(buffer,256);
 fgets(buffer,255,stdin);
 n = write(sockfd,buffer,strlen(buffer));
 if(strcmp(buffer,"EXIT\n") == 0)
 {
 printf("Connection Terminated\n");
 break;
 }
 if (n < 0)
 {
 error("ERROR writing to socket");
 }
 bzero(buffer,256);
 n = read(sockfd,buffer,255);
 printf("%s\n",buffer);
 if (n < 0)
 {
 error("ERROR reading from socket");
 printf("%s\n",buffer);
 }
}while(1 == 1);
close(sockfd);
return 0;
}

• जलजनक: use a debugger and step through. use fprintf(stderr, "") instead. Detach the new thread. Thread params ought to be individually allocated unless necessary.
• selbie: In your server thread_func, read() will return 0 to indicate the remote side cleanly closed a connection. As you have it now, I could write a rogue client that connects, then closes without sending "EXIT", and your thread will not exit.
• selbie: Well... it will probably exit on the subsequent "write" call.
• selbie: You just need to call "listen" once. So make that call prior to entering the do-loop.
• alk: Please format the sources properly. Currently it's really difficult to read. Reading tnis isn't fun.

Answer 1

Two errors:

1. You are casting too much, the only place here should be the inaddr stuff.
2. You are not listening to your compiler, crank up the warning level.

Now, the problem (or maybe just one?) is actually this:

pthread_create(&pth,NULL,threadFunc(newsockfd),(void*) &i);

This will call threadFunc(newsockfd) and pass the result to pthread_create. The second part will never happen though, because that function calls pthread_exit or falls off the end without returning anything, which could cause anything to happen.

• Martin James: Yeah - fix @Ulrich UB by passing just 'threadFunc' so that the function address gets used and then pass the new socket fd as the last (void*) parameter, instead of that 'i' thingy, (not sure what that is for?).

Answer 2

Your server code isn't displaying the printf statements reliably is because you didn't end the strings passed to printf with a "\n".

Change all of your printf statements to include a trailing \n such that output will be "flushed" immediately. E.g.

Instead of:

printf("after pthread create");

Do this:

printf("after pthread create\n");

Repeat that fix for all of your printf statements. And then the program flow will be more readily visible as clients connect to it.

There's probably about 5 or 6 other bugs in your code. The main one that I want to call out is just because the client sent 4 bytes of "EXIT", doesn't mean the TCP stream won't fragment that into "EX" and "IT" across two seperate read calls depending on the state of the intertubes. Always write your protocol code as if read/recv were only going to return one char at a time. OR just use MSG_WAITALL with recv() so that you always read the chunk size.