Sign in to follow this  
simotix

Simple threaded TCP Server using to much CPU

Recommended Posts

simotix    142
I have created a simple threaded tcp server, however, at idle it is using way to much cpu (like 22%). I have tried to sleep half a second ... two seconds, nothing is working on slower it down. I was hoping that someone could suggest a way on getting such a simple application to stop pulling so much cpu power. I am on windows and I am using winsock. main.cpp
#include <winsock2.h> /* Networking include */
#pragma comment(lib, "Ws2_32.lib") // For winsock<2>.

#include <process.h> /* Threading include */
#include <vector>
using std::vector;

#include <iostream>
using std::cout;
#include <conio.h> /* For _kbhit() */
#include "net_common.h"
#include "server.h"

struct tApplicationInfo
{
	vector<SOCKET> vConnectedSockets;
	SOCKET skServerSocket;
	bool bRunning;
};

unsigned __stdcall ThreadProcAcceptNewSockets(void *pVoid);
unsigned __stdcall ThreadProcReceiveFromSockets(void *pVoid);

int main(void)
{
	if (!NET_Init())
	{
		// Could not load the winsock .dll.
		exit(EXIT_FAILURE);    
	}

	HANDLE hThreadAcceptNewSocketsHandle = NULL;
	HANDLE hThreadReceiveFromSocketsHandle = NULL;
	unsigned int unThreadAcceptNewSocketsID = 0;
	unsigned int unThreadReceiveFromSocketsID = 0;

	tApplicationInfo appInfo;
	appInfo.bRunning = true;

	CreateServerConnection(27000, appInfo.skServerSocket);

	{
		hThreadAcceptNewSocketsHandle = reinterpret_cast<HANDLE>(_beginthreadex(NULL, 0, ThreadProcAcceptNewSockets, (void *)&appInfo, 0, &unThreadAcceptNewSocketsID));

		if(hThreadAcceptNewSocketsHandle == NULL)
			cout << "Failed to create the thread \"Accept New Sockets\"." << GetLastError() << endl;
	}

	{ 
		hThreadReceiveFromSocketsHandle = reinterpret_cast<HANDLE>(_beginthreadex(NULL, 0, ThreadProcReceiveFromSockets, (void *)&appInfo, 0, &unThreadReceiveFromSocketsID));

		if(hThreadReceiveFromSocketsHandle == NULL)
			cout << "Failed to create the thread \"Accept From Sockets\"." << GetLastError() << endl;
	}

	while(true)
	{
		if(_kbhit() != 0)
		{
			appInfo.bRunning = false;
			break;
		}
	}

	if(hThreadAcceptNewSocketsHandle != NULL)
	{
		WaitForSingleObject(hThreadAcceptNewSocketsHandle, INFINITE);
		CloseHandle(hThreadAcceptNewSocketsHandle);
	}

	if(hThreadReceiveFromSocketsHandle != NULL)
	{
		WaitForSingleObject(hThreadReceiveFromSocketsHandle, INFINITE);
		CloseHandle(hThreadReceiveFromSocketsHandle);
	}

	NET_ShutDown();

	return 0; 
}

unsigned __stdcall ThreadProcAcceptNewSockets(void *pVoid)
{
	tApplicationInfo *pAppInfo = (tApplicationInfo *)pVoid;

	while(pAppInfo->bRunning == true)
	{
		AcceptNewSockets(pAppInfo->skServerSocket, pAppInfo->vConnectedSockets);
		Sleep(2000);
	}

	return 0;
}

unsigned __stdcall ThreadProcReceiveFromSockets(void *pVoid)
{
	tApplicationInfo *pAppInfo = (tApplicationInfo *)pVoid;

	while(pAppInfo->bRunning == true)
	{
		ReceiveFromSockets(pAppInfo->vConnectedSockets);

		Sleep(2000);
	}

	return 0;
}


Accept function
void AcceptNewSockets(SOCKET skServerSocket, vector<SOCKET> &vConnectedSockets)
{
	sockaddr_in clAddress;
	int  iClientAddrLen = sizeof(clAddress);

	SOCKET clSocket = accept(skServerSocket, (sockaddr *)&clAddress, &iClientAddrLen);
	// Wait for a client to connect.
	if (clSocket == INVALID_SOCKET)
	{
	//	cout << "accept() FAILED\n";
	//	DieWithError(NET_GetErrorString());
	}
	else
	{
		cout << "<Server> Client " << inet_ntoa(clAddress.sin_addr) 
			<< ":" << ntohs(clAddress.sin_port) << " connected.\n";
		vConnectedSockets.push_back(clSocket);
	}
}


Receive function
void ReceiveFromSockets(vector<SOCKET> &vConnectedSockets)
{
	// Receive the client message.
	int sizeOfStruct = sizeof(tNetworkStructTest);

	for(unsigned int i = 0; i < vConnectedSockets.size(); ++i)
	{
		if(i == 1)
			int a = 0;
		tNetworkStructTest structReceived;
		int iRecvMsgSize = 0;

		while(iRecvMsgSize < sizeOfStruct && iRecvMsgSize != -1)
		{
			iRecvMsgSize += recv(vConnectedSockets[i], reinterpret_cast<char *>(&structReceived), sizeOfStruct, 0);
		}
		////if (iRecvMsgSize == SOCKET_ERROR)
		////{
		////	cout << "recv() FAILED\n";
		////	DieWithError(NET_GetErrorString());
		////}
		if(iRecvMsgSize != -1)
		{
		cout << "<Client> " << structReceived.sShort << '\n';
		cout << "<Client> " << structReceived.szStuff << '\n';
		cout << "<Client> " << structReceived.unType << '\n';
		}
	}
}


Share this post


Link to post
Share on other sites
Kylotan    10011
This is bad code :(

Firstly, threaded sockets are almost always the wrong way to do it in C++. It makes sense in Java (for a small number of sockets) because the library provides the classes for you, but in C++ it gives you no benefits. Instead of trying to periodically read a socket, sleeping in the meantime, and using threads to pretend that the application is still able to proceed, you should use one of the various ways in which the operating system can inform you that the socket is ready for use. The select() function is the simplest way to do this.

Secondly, your code isn't thread-safe. You can't share vConnectedSockets across threads like that, as it makes no guarantees that the operations on it are atomic. At some point you'll end up reading an invalid socket or something like that and the whole thing will blow up.

Thirdly, the amount of CPU used is not proportional to the complexity of the program. A simple while(1) {} would consume all your CPU, for example. What matters is that if you keep calling a function over and over again then the CPU will be doing a lot of work. This is why you want to wait for the OS to tell you when it's time to read or write something, not to keep trying until it works.

Fourthly, your receive logic is not optimal. These are streamed sockets and therefore you cannot guarantee that if the other side has sent X bytes that you will necessarily have all X bytes ready to read. It's good that you recognise that you will require multiple recv calls, but unfortunately doing this in a tight loop like that will often waste time reading nothing from a socket that is waiting for more data to come in. During that time, you could have been reading data from other sockets that -do- have data ready. So one slow socket ends up holding up the rest, and your application sits in a busy loop using CPU time and doing nothing useful. The usual fix for this is to contain a buffer in your application for each socket and fill that from the recv calls. When there's nothing left to recv for one socket, move onto the next ready socket, even if you haven't received all the data yet. Repeat this enough times and eventually the buffer for a socket will have all the required data and you can process it as expected.

Share this post


Link to post
Share on other sites
simotix    142
The fact that the code I built is not correct or optimal does not come as a big surprise to me as it is my first application with networking or threading.

The reason I decided to use threading was because originally I had a blocking accept, however, once I changed it to no longer use blocking threading was not necessary. I have now removed threading and put everything closer together which leaves the CPU usage at about 0% - 1%.

However, as far as using select() I am still a bit fuzzy on the subject. I am still trying to wrap my head around the details of select, however, the way I get new connections should not change, should it?

Here is my main loop now

while(true)
{
if(_kbhit() != 0)
{
break;
}

AcceptNewSockets(skServerSocket, vConnectedSockets);
ReceiveFromSockets(vConnectedSockets);

Sleep(500);
}

Share this post


Link to post
Share on other sites
hplus0603    11356
The problem with your main loop is that every request will have 500 milliseconds of latency.

select() is the best option for simple programs with up to, say, 50 sockets. You can put your accept-ing socket into the select() call, and select() will tell you that the socket is readable when there is a connection to accept.

select() is very simple:

1) Put all sockets you want to read/accept from into an fd_set
2) Call select()
3) When select() returns, test each socket you want to read/accept from whether it's still in the fd_set
4) If it's there, you can call recv() or accept() *once* on that socket, and be guaranteed to not block
5) Repeat

Most of the socket tutorials on the web (including Beej's) should describe this for you.

If you're using C/C++, though, you might also want to look at boost::asio. It's a library that does asynchronous and/or threaded I/O for you, more or less automatically. You end up writing your code as a series of callback functions using boost::bind(). The tutorials are pretty good; I suggest you read through them and see if you like it!

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this