Async chat server/client with select()

[netpumber]

hello out there.

im trying to create a simple chat server/client with select method in C using Visual Studio 2010.
Clients will connect to the server and the server will send the message of each client to the rest.

We talk about a console app.

and here is the code.

Server:

#include <WinSock2.h>
#include <stdio.h>
#include <time.h>



main()
{

	SOCKET			ListeningSocket;
	SOCKET			AcceptSocket;

	SOCKADDR_IN		ServerAddr;
	SOCKADDR_IN		ClientAddr;

	WSADATA			wsaData;

	const unsigned short PORT = 4444;

	FD_SET			fdread;
	FD_SET			BackUpfdread;
	FD_SET			fdwrite;
	FD_SET			BackUpfdwrite;

	int				maxDescriptor;
	SOCKET			SocketArray[20];
	int				index = 0;
	int				selectResults;
	int				i,k;
	int				clientAddrSize;
	int				RecvBytes;
	int				SentBytes;

	char			SentBuff[500];
	char			RecvBuff[500];

	struct	timeval timeout;


	// Initialize Winsock2.2
	WSAStartup(MAKEWORD(2,2),&wsaData);

	// Initialize Listening Socket
	ListeningSocket = socket(AF_INET,SOCK_STREAM,0);


	// Initialize ServerAddr
	ServerAddr.sin_family = AF_INET;
	ServerAddr.sin_addr.s_addr = htonl(INADDR_ANY);
	ServerAddr.sin_port = htons(PORT);

	// Bind the ServerAddr with ListeningSocket
	bind(ListeningSocket,(SOCKADDR *)&ServerAddr,sizeof(ServerAddr));

	// Listening Socket
	listen(ListeningSocket,5);

	FD_ZERO(&fdread);
	FD_ZERO(&BackUpfdread);
	FD_ZERO(&fdwrite);
	FD_ZERO(&BackUpfdwrite);

	// Asign ListeningSocket at fdread
	FD_SET(ListeningSocket,&fdread);

	maxDescriptor = ListeningSocket;

	SocketArray[index] = ListeningSocket;
	index++;

	timeout.tv_sec = 0;
	timeout.tv_usec = 0;

	// Main loop starts here
	for(; ;)
	{
		
		BackUpfdread = fdread;
		BackUpfdwrite = fdwrite;
		selectResults = select(maxDescriptor+1,&BackUpfdread,&BackUpfdwrite,NULL,&timeout);
		//printf("server select() OK\n");

		if(selectResults == -1)
		{
			printf("Select() error");
			WSACleanup();
			return 0;
		}


		for(i=0;i<=index-1;i++)
		{
			//printf("%d\n",SocketArray[i]);
			if(FD_ISSET(SocketArray[i],&BackUpfdread))
			{
				if(SocketArray[i] == ListeningSocket) // we have a new connection
				{
					clientAddrSize = sizeof(ClientAddr);
					AcceptSocket = accept(ListeningSocket,(SOCKADDR *)&ClientAddr,&clientAddrSize);

					// Add the newest accepted socket to the fdread and fdwrite sets.
					FD_SET(AcceptSocket,&fdread);
					FD_SET(AcceptSocket,&fdwrite);

					// Add the newest accepted socket into SocketArray
					SocketArray[index] = AcceptSocket;
					index++;

					// keep track of the maxDescriptor.
					if(AcceptSocket > maxDescriptor)
					{
						maxDescriptor = AcceptSocket;
					}

					printf("New connection from %s on socket %d\n", inet_ntoa(ClientAddr.sin_addr), AcceptSocket);

				}else{ // That means that the socket is not from a new conection and has something sent.

					memset(RecvBuff,0,sizeof(RecvBuff));
					RecvBytes = recv(SocketArray[i], RecvBuff, sizeof(RecvBuff)-1, 0);

					if(RecvBytes > 0) // Some data received.
					{

						printf("Message Send.\n");
						printf("Message was: %s\n",RecvBuff);

						for(k=0;k<=index-1;k++)
						{
							if(FD_ISSET(SocketArray[k],&BackUpfdwrite))
							{
								if(SocketArray[k] != ListeningSocket && SocketArray[k] != SocketArray[i])
								{
									memset(SentBuff,0,sizeof(SentBuff));
									strcpy(SentBuff,RecvBuff);
									SentBytes = send(SocketArray[k],SentBuff,sizeof(SentBuff),0);

								}

							}
						}

					}

				}

			}
		}
		SleepEx(10, FALSE);

	}// Main loop ends here


}

Client :

#include <WinSock2.h>
#include <stdio.h>
#include <time.h>



main()
{

	SOCKET			ConnectSocket;
	SOCKET			SocketArray[20];

	SOCKADDR_IN		ServerAddr;

	WSADATA			wsaData;

	FD_SET			fdwrite;
	FD_SET			fdread;
	FD_SET			BackUpfdread;
	FD_SET			BackUpfdwrite;

	char			server_address[20] = "192.168.1.65";
	char			SentBuff[500];
	char			RecvBuff[500];

	const unsigned short PORT = 4444;

	int				maxDescriptor;
	int				index = 0;
	int				SelectResults;
	int				i;
	int				RecvBytes;
	int				SentBytes;

	BOOL			bOpt = TRUE;

	struct timeval	timeout;


	// Initialize Winsock 2.2
	WSAStartup(MAKEWORD(2,2),&wsaData);

	// Initialize ServerAddr
	ServerAddr.sin_family = AF_INET;
	ServerAddr.sin_addr.s_addr = inet_addr(server_address);
	ServerAddr.sin_port = htons(PORT);

	// Create a new socket to make a client connection.
	ConnectSocket = socket(AF_INET,SOCK_STREAM,IPPROTO_TCP);

	setsockopt(ConnectSocket, IPPROTO_TCP, TCP_NODELAY,(char*)&bOpt,sizeof(BOOL));

	// Clear the fd sets
	FD_ZERO(&fdread);
	FD_ZERO(&BackUpfdread);
	FD_ZERO(&fdwrite);
	FD_ZERO(&BackUpfdwrite);

	// Asign ConnectSocket into fdread and fdwrite.
	FD_SET(ConnectSocket,&fdread);
	FD_SET(ConnectSocket,&fdwrite);

	// Set timer
	timeout.tv_sec = 0;
	timeout.tv_usec = 0;

	maxDescriptor = ConnectSocket;
	SocketArray[index] = ConnectSocket;
	index++;

	// Make a connection to the server with socket s.
	if(connect(ConnectSocket, (SOCKADDR *) &ServerAddr, sizeof(ServerAddr)) == SOCKET_ERROR)
	{
		printf("Couldn't connect to the server\n");
	}


	// Main loop starts here
	for(; ;)
	{
		BackUpfdread = fdread;
		BackUpfdwrite = fdwrite;

		memset(SentBuff, 0, sizeof(SentBuff));
		printf("Write: ");
		gets_s(SentBuff, sizeof(SentBuff));

		

		SelectResults = select(maxDescriptor+1,&BackUpfdread,&BackUpfdwrite,NULL,&timeout);

		for(i=0;i<=index-1;i++)
		{
			// Something to read from server.
			if(FD_ISSET(SocketArray[i],&BackUpfdread) && SocketArray[i] == ConnectSocket) 
			{
				RecvBytes = recv(SocketArray[i], RecvBuff, sizeof(RecvBuff), 0);

				if(RecvBytes > 0)
				{
					printf("%s\n",RecvBuff);   
					// Cleaning the Receive Buffer 
					memset(RecvBuff,0,sizeof(RecvBuff));
				}

			}

			// Something to write.
			if(FD_ISSET(SocketArray[i],&BackUpfdwrite) && SocketArray[i] == ConnectSocket)
			{
				SentBytes = send(SocketArray[i], SentBuff,sizeof(SentBuff),0);
				// Cleaning the Sent Buffer 
				memset(SentBuff,0,sizeof(SentBuff));
				
			}


		}

		SleepEx(10, FALSE);
		
	} // Main menu ends here

}

The problem is the delay of messages' transfer, to clients. Can somebody find where the mistake is with my app ?

Any suggestion is welcomed.

Thanks.

[MikeAThon]

On the server side, as soon as you receive data from a client, you should call send() to respond. You do not need to check on readyness to write (i.e., you do not need to wait until FD_ISSET for writing). The only time you need to check on readyness to write is if you call send() and send() returns an indication that the socket is not ready to write.

In addition, your code assumes that all data (including a terminating NULL) will be received in a single call to recv(), and further assumes that all data will be sent in a single call to send(). Both are bad assumptions. TCP is a stream-based protocol, not a message-based protocol, so when using TCP, you must implement some sort of serialization when sending and receiving bytes.

Mike

[netpumber]

First of all thank you for your answer.

I ve change the piece of server's code that sends the message to the clients removing the

if(FD_ISSET(SocketArray[k],&BackUpfdwrite))

now the server became like this :

}else{ // That means that the socket is not from a new conection and has something sent.

					memset(RecvBuff,0,sizeof(RecvBuff));
					RecvBytes = recv(SocketArray[i], RecvBuff, sizeof(RecvBuff)-1, 0);

					if(RecvBytes > 0) // Some data received.
					{

						printf("Message Sent.\n");
						printf("Message was: %s\n",RecvBuff);

						for(k=0;k<=index-1;k++)
						{
							
							if(SocketArray[k] != ListeningSocket && SocketArray[k] != SocketArray[i])
							{
								memset(SentBuff,0,sizeof(SentBuff));
								strcpy(SentBuff,RecvBuff);
								SentBytes = send(SocketArray[k],SentBuff,sizeof(SentBuff),0);

								if(SentBytes > 0)
								{
									printf("Message Forwarded\n");
								}else{
									printf("Message forward error\n");
								}

							}

							
						}

					}

the problem still exists with the delay of message transfer and this means that there is something else that make messages not be transferred on time.

How you find client's code ? Does it need select and fd_isset too ? or i have to code it in other way?

If i connect to the server two telnet clients seems to work nice and that's why i m thinking that maybe the problem is in client.

Also can you make more clear your words in the second paragraph cuz didn't understood them very well.

[MikeAThon]

How you find client's code ? Does it need select and fd_isset too ? or i have to code it in other way?

If i connect to the server two telnet clients seems to work nice and that's why i m thinking that maybe the problem is in client.

In almost all situations, client or server, you do not need to check for readiness to write before calling send(). In other words, you do not need to wait until FD_ISSET before writing. The only time you need to check on readiness to write is if you call send() and send() returns an indication that the socket is not ready to write. If that happens, then you must wait before calling send() again until FD_ISSET.

So, in general, the algorithm is:
1 - call send()
2 - did send() complete correctly? If so, then you're done; call send or recv or whatever for new data.
3 - If send() did not complete correctly, call select on a write_fd_set and then call send() once it returns with FD_ISSET

Look here for a basic client program, which uses blocking sockets (like you do): "Winsock Programmer’s FAQ - Basic Winsock Examples: Basic Blocking Client " at http://tangentsoft.net/wskfaq/exampl...ic-client.html

Other examples from the above site start here: http://tangentsoft.net/wskfaq/exampl...ics/index.html

Also can you make more clear your words in the second paragraph cuz didn't understood them very well.

"TCP is a stream-based protocol" means that the data on the wire is a stream of individual bytes: there is no notion of packets. If you call send() quickly, three times in a row, with 50 then 100 then 150 bytes, the result will be a stream of 300 bytes on the wire. The recipient must somehow decode these into the original three calls to send() (if that's the intention). Likewise, on the receiving end, one call to recv() might result in receipt of bytes from multiple calls to send() and indeed it's possible that the recipient receives only part of the bytes from a single call to send().

For that reason, it's up to you, as the application programmer, to institute some sort of application-level protocol in order to serialize and de-serialize the data being sent.

One protocol is for the server always to terminate sent bytes with a NULL when calling send(). It's wrong, however, for the client to assume that a single call to recv() will always contain the terminating NULL that was sent from the server; rather, the client must actively look for it, and continue to call recv() until the terminating NULL is found. It also cannot disregard bytes recieved after the terminating NULL, since those bytes might contain the beginning of a new NULL-terminated message from the server.

Mike

[MikeAThon]

Incidentally, I just realized that your client is using a combination of blocking sockets and select(). That makes no sense, and as you have realized, your call to select() must include a timeout or else it might never return.

Use of select() makes sense only when using non-blocking sockets.

You have set the timeout to zero, which means that you are constantly polling for activity, without any pause. That's why you also needed the SleepEx call for 10 msec, since otherwise you would be using 100% CPU. Both are bad (meaning both of polling and SleepEx to avoid 100% CPU are bad).

See better designs at the Winsock FAQ site that I linked to.

Mike