Sign in to follow this  
weasalmongler

Feedback on network code

Recommended Posts

Hi everyone. I'm pretty new to network programming, and while I understand the concepts I wanted to know what people thought of my current TCP network code.

#define PORT_USED 7749	//Completely random, change if it conflicts
#define MAX_CLIENTS 32


typedef struct SNetDataHeader
{
	int					clientid;				//Client message is from
	int					type;					//Type of request, eg connect, disconnect
	int					target;					//Target id of request if relevant
	int					datasize;				//Size of the data sent from the client
} netDataHeader;


class CNetwork
{
public:
	CNetwork();
	~CNetwork();
	
	//Members
	int 	CreateLocalServer();			//Returns listening socket
	int	AcceptConnection(int socketid);	//Returns client socket, called once a frame
	
	int	ConnectTo(std::string ip);		//Returns server socket
	void	Disconnect(int socketid);
	
	int	Send(int socketid, char* buffer, int bufferlen );
	int	Receive(int socketid, char* buffer, int bufferlen );

protected:
	
};




CNetwork::CNetwork()
{
    //Initialize winsock
    WSADATA wsaData;
    WORD version;
    int error;

    version = MAKEWORD( 2, 0 );

    //Startup winsock
    error = WSAStartup( version, &wsaData );
    if ( error != 0 )
    {
        g_cLog.WriteError(ERROR_CRITICAL, "CNetwork::CNetwork - Winsock 2 could not be initialized!");
        return;
    }
    
    //Check for correct version
    if ( LOBYTE( wsaData.wVersion ) != 2 || HIBYTE( wsaData.wVersion ) != 0 )
    {
        g_cLog.WriteError(ERROR_CRITICAL, "CNetwork::CNetwork - Winsock 2 version incorrect!");
        WSACleanup();
        return;
    }

    //Winsock successfully initialized

}


CNetwork::~CNetwork()
{
    //Cleanup winsock
	WSACleanup();
}



int CNetwork::CreateLocalServer()
{
	struct sockaddr_in	server_address;
	int server_socket;
	
	//Create the TCP socket
//	server_socket = socket(PF_INET, SOCK_STREAM, 0);
    server_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
	if(server_socket == INVALID_SOCKET)
	{
		g_cLog.WriteError(ERROR_WARNING, "CNetwork::CreateLocalServer - Could not create socket!");
		return -1;
	}
    //Turn off delay waiting to fill a MTU of data before sending
   	int flag = 1;
   	setsockopt(server_socket, IPPROTO_TCP, TCP_NODELAY, (char *) &flag, sizeof(int));

	//Make port non blocking
    u_long NonBlock = 1;
    if (ioctlsocket(server_socket, FIONBIO, &NonBlock) == SOCKET_ERROR)
    {
        g_cLog.WriteError(ERROR_WARNING, "CNetwork::CreateLocalServer - Could not set non blocking mode");
        return -1;
    }
	
	//Bind the server to the socket
	server_address.sin_addr.s_addr = INADDR_ANY;
	server_address.sin_family = AF_INET;
	server_address.sin_port = htons(PORT_USED);
	memset(server_address.sin_zero, 0, sizeof(server_address.sin_zero));
	
	if(bind(server_socket, (struct sockaddr*)&server_address, sizeof(server_address)) < 0)
	{
		g_cLog.WriteError(ERROR_WARNING, "CNetwork::CreateLocalServer - Could not bind socket!");
		return -1;
	}

    if(listen(server_socket, 8) == SOCKET_ERROR)
	{
		g_cLog.WriteError(ERROR_WARNING, "CNetwork::CreateLocalServer - Could not listen to socket!");
	}
	
	return server_socket;
}


int	CNetwork::AcceptConnection(int socketid)		//Called once a frame
{
	int socketsize;
	struct sockaddr_in  client_address;
	int returnsock;
	
	socketsize = sizeof(client_address);
	returnsock = accept(socketid, (struct sockaddr*)&client_address, &socketsize);
	if(returnsock != INVALID_SOCKET)
    {
        //Turn off delay waiting to fill a MTU of data before sending
        int flag = 1;
        setsockopt(returnsock, IPPROTO_TCP, TCP_NODELAY, (char *) &flag, sizeof(int));

        u_long NonBlock = 1;
        if (ioctlsocket(returnsock, FIONBIO, &NonBlock) == SOCKET_ERROR)
        {
            g_cLog.WriteError(ERROR_WARNING, "CNetwork::AcceptConnection - Could not set non blocking mode");
            return -1;
        }
    }

	return returnsock;
}


int CNetwork::ConnectTo(std::string ip)
{
	int newsocket;
	struct sockaddr_in  server_address;
	
//	newsocket = socket(PF_INET, SOCK_STREAM, 0);
    newsocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
	if(newsocket == INVALID_SOCKET)
	{
		g_cLog.WriteError(ERROR_WARNING, "CNetwork::ConnectTo - Failed to create socket!");
		return -1;
	}

    //Turn off delay waiting to fill a MTU of data before sending
   	int flag = 1;
   	setsockopt(newsocket, IPPROTO_TCP, TCP_NODELAY, (char *) &flag, sizeof(int));
	
	server_address.sin_addr.s_addr = inet_addr(ip.data());
	server_address.sin_family = AF_INET;
	server_address.sin_port = htons(PORT_USED);
	memset(server_address.sin_zero, 0, sizeof(server_address.sin_zero));
	
	if( connect(newsocket, (struct sockaddr *) &server_address, sizeof(server_address)) < 0 )
	{
		g_cLog.WriteError(ERROR_WARNING, "CNetwork::ConnectTo - Failed to connect to server.");
		return -1;
	}
	
	//Make port non blocking, doing this after the connect request makes it wait until connected
    u_long NonBlock = 1;
    if (ioctlsocket(newsocket, FIONBIO, &NonBlock) == SOCKET_ERROR)
    {
        g_cLog.WriteError(ERROR_WARNING, "CNetwork::ConnectTo - Could not set non blocking mode");
        return -1;
    }
		
	return newsocket;
}

void CNetwork::Disconnect(int socketid)
{
	if(socketid)
	{
		closesocket(socketid);
	}
}


int CNetwork::Send(int socketid, char* buffer, int bufferlen )
{
	return send(socketid, buffer, bufferlen, 0);
}


int CNetwork::Receive(int socketid, char* buffer, int bufferlen )
{
	return recv(socketid, buffer, bufferlen, 0);
}



Essentially the server does this each frame:
//This is pseudo code
//Get new connections
newsocket = network->AcceptConnection(this->serverid);

//Handle the new socket

for (each client)
{

    //Get data from each client
    int result = this->network->Receive(clientid, (char*)&data, sizeof(netDataHeader));
    if( result != SOCKET_ERROR )
    {
         //Do something with the message
    }

}


The client updates something like this:
int CGameClient::Update(float elapsed)
{
	//Read in all the new data from the stream
	CRequest* req;
	netDataHeader data;
    memset(&data, 0, sizeof(netDataHeader));

    int result = this->network->Receive(this->serverid, (char*)&data, sizeof(netDataHeader));
    if(result == SOCKET_ERROR)
    {
 //       this->network->LogError();
    }
    else
    {
        if(result > 0)
        {
	        req = new CRequest();
	        req->clientid = data.clientid;
	        req->type = data.type;
	        req->target = data.target;
	        req->datasize = data.datasize;
	        if(req->datasize > 0)
	        {
		        req->data = new char[req->datasize];
                memset(req->data, 0, req->datasize);
		        this->network->Receive(this->serverid, (char*)req->data, req->datasize);
	        }

	        //Log our client id
	        this->clientid = req->target;
    		
	        requestlist.push(req);
        }
    }
	
	return OK;
}


Does this look like a good system or are there big flaws with it? The problems I seem to have with it are that if I send lots of messages then there seems to be some lag. I've fixed this issue by limiting the server to update 25 times a second and implementing interpolation in the client, and also limiting the number of input messages that are sent from the clients. Also I seem to get a crash when I send small numbers of entities across the network.

Share this post


Link to post
Share on other sites
Quote:
int result = this->network->Receive(this->serverid, (char*)&data, sizeof(netDataHeader));


Where in your code are you re-assembling the messages. send/recv do not preserve the data boundaries. While this may work on LAN, it'll break fast on WAN.

With TCP, the result may be not error, but that doesn't mean you actually received sizeof(netDataHeader) bytes.

For TCP, you need loop on recv until you read at least sizeof(netDataHeader) bytes. Then you need to loop again until you receive entire payload.

On every frame you'd usually keep calling recv() until you get WOULD_BLOCK error. Whatever you receive you put into relevant buffers. Then, you scan these buffers to see if they contain entire header, then check if they contain entire payload. If they do, handle the message.

Also, in C++, it's common to do:

struct SNetDataHeader
{
int clientid; //Client message is from
int type; //Type of request, eg connect, disconnect
int target; //Target id of request if relevant
int datasize; //Size of the data sent from the client
};

SNetDataHeader netDataHeader; // allocate where needed



Quote:
#define PORT_USED 7749


Hardcoding this is somewhat annoying, what happens when you'll want to test your applications locally, and you'll run into a situation where you need to run multiple servers.

While not immediately relevant, reading directly into memory causes problems with alignment. Your struct may be 16 bytes in most cases, but depending on compiler and target architecture, it may be padded to 32 bytes, perhaps when compiled for 64-bit targets.

Quote:
if I send lots of messages then there seems to be some lag


Lag probably comes from invalid messages, or from Nagle algorithm (which can be disabled). If testing on LAN, or locally, there should be no problem transceiving megabytes of data.

Share this post


Link to post
Share on other sites
Thanks Antheus, that is a very informative post. You've cleared up some misconceptions I've obviously had with network programming, I've googled loads of tutorials but they all seem to be the same and none seem to give very good descriptions of things.

I'll go about improving my code based on some of your suggestions. Thanks again.

Share this post


Link to post
Share on other sites
Hi again.

I have made some changes to the code so now it should be much more reliable. It now buffers data properly when receiving and I have fixed all crashing issues. The only problem now is still the lag. I have switched off nagling using:


int flag = 1;
setsockopt(newsocket, IPPROTO_TCP, TCP_NODELAY, (char *) &flag, sizeof(int));


I do this for every socket that gets created (when creating a client socket, server socket or after an accept()). However, there is still always a quarter of a second or so where I press "forwards" and don't move forwards. Then I start moving and release the forwards key and it takes a quarter of a second to stop.

I have checked that it is not being flooded with packets, a couple of packets arrive every few frames both on server and client (I have fixed server updates at 60FPS). I am running it across a 100Mbit lan, so it should be running much faster than this, each packet is very small. Does anyone have any ideas?

Thanks.

Share this post


Link to post
Share on other sites
Even though the server and client is on the same LAN (or locally), are you connecting locally? That is, are you connecting to localhost (127.0.0.1) or your public IP? Still, 250ms latency does seem quite high. Might want to also make sure that your buffer is working properly. I've had cases where a slight oversight with the buffering resulted in a send queue with just one item wouldn't send, making it seem laggy when in reality I was just not always sending right away.

Share this post


Link to post
Share on other sites
Well, I don't have any problem on the local computer, the lag only occurs for other computers connecting across the network. The machine running the server connects to 127.0.0.1 or 192.168.0.2 (the servers IP) and runs absolutely fine. The lag problem only occurs on the other machine (which isn't running a server) which also connects to 192.168.0.2.

I'm pretty sure my receive buffering is working fine, and should process as many messages as it can in a frame. Did you mean it might be a problem with the sending code? When I send at the moment I simply memcpy a header structure and its corresponding data chunk into a continuous block of memory and then send it with a single send() command.

Share this post


Link to post
Share on other sites
Sorry, let me clarify - the problem I was saying I had was with a send queue, but I was saying you might want to check all your buffering/queueing, both on the client and server, both with sending and receiving, to make sure that they performing like you think they should be. You can write up a quick logging of the times which certain events happen, such as:

time -> message
00012 -> server: sent Move
00015 -> client: received data, placed in buffer
00015 -> client: read buffer, received Move
00015 -> client: queued data for handling
00025 -> client: processed received data

...etc. That way you can find out exactly where it is taking so long instead of just assuming where it is. If the times look fine, you might want to also output the data sent/received to make sure it matches up, ensuring you aren't somehow losing data somewhere.

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