Jump to content
  • Advertisement
Sign in to follow this  
erpeo93

Network Library API critique

Recommended Posts

Posted (edited)

Hi,

 

as part of my game development, I've developed a UDP-based network library for the client-server communication, written from scratch because I was unable to find a good one that was simple enough to use.

Now that the library has been decently tested, I think that it could be valuable to start making it an actual "library" that someone could download and use.

As a preliminary step, here's the current state of the API, I would really love to have some feedbacks from the community on this, especially from people that has some kind of experience with network programming.

Here it is, it should be pretty easy to understand, but of course feel free to ask anything if it's not clear :)

 

NetworkInterface* Network_Client_Open_Connection(char* IP, int port);

void Network_Close_Connection(NetworkInterface* network, int connectionSlot);

NetworkInterface* Network_Server_Init(int port, int maxConnectionCount);

//returns how many connections were accepted.
int Network_Accept(NetworkInterface* network, int* acceptedSlots, int maxAccepted);


enum SendPacketChannel

{
SendPacket_NoGuarantees,
SendPacket_DeliveryGuaranteed,
SendPacket_Ordered,
};
void Network_Queue_Packet(NetworkInterface* network, int connectionSlot, SendPacketChannel channel,void* data, int size);

bool Network_Flush_Send_Queue(NetworkInterface* network, int connectionSlot, float elapsedTimeSinceLastFlush);

void Network_Receive_Raw_Data(NetworkInterface* network);

#define MAX_PACKET_SIZE 1024
struct NetworkPacketReceived
{
    bool disconnected;
    int dataSize;
    unsigned char data[MAX_PACKET_SIZE];

};
NetworkPacketReceived Network_Get_Packet(NetworkInterface* network, int connectionSlot);





///////////////////////////EXAMPLE///////////////////////


SERVER:

#define NET_IMPLEMENTATION
#include "net.h"

int main()
{
  	int port = 1111;
  	int maxConnections = 1024;
  
  	NetworkInterface* serverNetwork = Network_Server_Init(port, maxConnections);
	while(true)
	{
		float elapsed = Tick();
      
      	int acceptedSlots[16];
		int accepted = Network_Accept(NetworkInterface* network, acceptedSlots, ArrayCount(acceptedSlots));
      
      	for(int acceptedIndex = 0; acceptedIndex < accepted; ++acceptedIndex)
        {
        	//This is all application code!
          	Player* player = GetNewPlayer();
          	player->slot = acceptedSlots[acceptedIndex];
        }
      
      	Network_Receive_Raw_Data(serverNetwork);
      
      	for(everyPlayer)
        {
        	Player* player = GetPlayer();
          
          	
          	while(true)
          	{
          		NetworkPacketReceived packet = Network_Get_Packet(serverNetwork, player->slot);
            
            	if(!packet.size)
            	{
            		if(packet.disconnected)
                	{
                		Network_Close_Connection(serverNetwork, player->slot);
                	}
              	
              	break;
            	}
            	else
            	{
           			//Do Something with the received packet, in this case we just slam the packet back to the client!
            		Network_Queue_Packet(serverNetwork, player->slot, SendPacket_NoGuarantees, packet.data, packet.size);
            	}
          	}
          
            bool allPacketSent = Network_Flush_Send_Queue(serverNetwork, player->slot, elapsed);
        }
      	
	}
  	

}

///////////////////////////////////////////
CLIENT
  
#define NET_IMPLEMENTATION
#include "net.h"
int main()
{
	int port = 1111;
  	char* server = "127.0.0.1";
  
  	NetworkInterface* clientNetwork = Network_Client_Open_Connection(server, port);
	while(true)
	{
		float elapsed = Tick();
      	Network_Receive_Raw_Data(clientNetwork);
      
         while(true)
         {
         	NetworkPacketReceived packet = Network_Get_Packet(clientNetwork, 0);
            if(!packet.size)
            {
            	if(packet.disconnected)
                {
                	Network_Close_Connection(clientNetwork, 0);
                }
              	
              	break;
            }
            else
            {
           		//Do Something with the received packet!
              	printf(packet.data);
            }
          }
          
      		char toSend[256];
      		sprintf(toSend, "hello world, this is packet number: %d", packetIndex);    
			Network_Queue_Packet(clientNetwork, 0, SendPacket_Ordered, toSend, StrLen(toSend));
     
        	bool allPacketSent = Network_Flush_Send_Queue(clientNetwork, 0, elapsed);
    }
}

 

There are of course many things missing under the hoods (the library is unable to fragment big packets at the moment, and there's no secure connection, and other things, but those things shouldn't alter the API all that much... 

 

edit: sorry for the indentation... I'll try to make it a little more readable tomorrow...

 

Best regards,

Leonardo

Edited by erpeo93

Share this post


Link to post
Share on other sites
Advertisement

Some things that jump out: 

NetworkPacketReceived Network_Get_Packet

This is quite inefficient. It's better to receive into an existing packet buffer, and then use pointers-to-buffers to pass the data around, rather than copy the data. That in turn means that the client also needs some way of "releasing" a buffer when it's done, and perhaps also "getting" a buffer if you use the same structure for queuing outgoing data. It looks like that API is copying, too.

Generally, I like APIs better that use some type that generates type errors if I use it wrong, that just plain "int." So, the "connectionSlot" could probably be replaced with a "connection_t" which could be a "struct connection *". You don't need to actually define "struct connection," just keep the type distinct.

It's unclear, in NetworkPacketReceived, how I tell whether a message came over the unrealiable or reliable channels.

Generally, a server will want to get the IP address of clients, so it can implement block lists or such. Keeping that inside the library is probably going to make it hard on users of the library.

I don't see how you deal with IPv6 vs IPv4 -- do you use dual-stack getaddrinfo() calls in the implementation?

I don't see any affordance for setting the timeout of a client -- how long does the library wait before it suggests a client has timed out? What happens if a client then comes back and tries sending more data?

Share this post


Link to post
Share on other sites
Posted (edited)
7 hours ago, hplus0603 said:

Some things that jump out: 


NetworkPacketReceived Network_Get_Packet

This is quite inefficient. It's better to receive into an existing packet buffer, and then use pointers-to-buffers to pass the data around, rather than copy the data. That in turn means that the client also needs some way of "releasing" a buffer when it's done, and perhaps also "getting" a buffer if you use the same structure for queuing outgoing data. It looks like that API is copying, too.

This is true, at the moment an "extra" copy is done when receiving a packet to simplify the API and make it more readable, it would be very trivial to remove one of the copy by chaning the API adding a "Network_Release_Packet()" to be called after "Network_Get_Packet()".

I feel like it's impossible to receive packets with 0 copies after the recv, unless you give up on compression and security... Am I wrong?

 

On the sending side of things I'm also doing a double copy before the actual send, one is done the moment you queue the packet, and then every time the API decides that the packet has to be resent, it does another copy into the actual sendbuffer, on which the actual send is called. (This happens because every time the library sends a packet it also includes additional informations such as acks etc. This second copy could be removed as well by "preallocating" all the extra space needed for the additional information. 

 

I feel like at that point we would be talking about one copy from the recv to the actual application dispatch, and same for sending... which would seems reasonable...?

 

Quote

Generally, I like APIs better that use some type that generates type errors if I use it wrong, that just plain "int." So, the "connectionSlot" could probably be replaced with a "connection_t" which could be a "struct connection *". You don't need to actually define "struct connection," just keep the type distinct.

True... I'm an idiot because that also allows me to "hide" the implementation details of the "connection", so that if in the future I want to change what happens under the hoods, I can without breaking any program.

Quote

It's unclear, in NetworkPacketReceived, how I tell whether a message came over the unrealiable or reliable channels.

At the moment it's not included because I ended up not needing it for my game, but would be absolutely trivial to implement :)

Also I feel like this could be a good discussion point... at the beginning I thought that the library should allow the user to define an arbitrary number of channels, and each channel chould potentially have set some properties like compression, encryption, etc

But then It seemed a little too complicated, and I made a step back on that...

 

Quote

Generally, a server will want to get the IP address of clients, so it can implement block lists or such. Keeping that inside the library is probably going to make it hard on users of the library.

True.. the "accept" function should also return that as well, should be pretty easy.

Also regarding the accept call, I was thinking that maybe written in the following way would be easier to use and read?

 

bool acceptingConnections = true;

while(acceptingConnections)
{
   NetworkConnection* connection = NetworkAccept(network);
   if(connection)
   {
      char IP[64];
      NetworkGetIP(connection, IP, sizeof(IP));
      //DoSomethingWithTheConnection();
   }
   else
   {
       acceptingConnections = false;
   }
}      	

 

Quote

I don't see how you deal with IPv6 vs IPv4 -- do you use dual-stack getaddrinfo() calls in the implementation?

I thought that by specifing AF_UNSPEC when calling getaddrinfo() would return both v4 and v6 addresses?

What you mean is that the API should allow the user to "specify" if he wants to make things in v4 or in v6?

 

Quote

I don't see any affordance for setting the timeout of a client -- how long does the library wait before it suggests a client has timed out? What happens if a client then comes back and tries sending more data?

At the moment the timeout is hardcoded, but yes I will need something like:

NetworkSetParameter(network, NetworkParam_Timeout, 30.0f);

Which doesn't seems to have any impact on the "core" part of the api, fortunately.

This would also makes sense for setting things like sending and receiving buffer dimension.

 

 

Many thanks for the very useful feeback :)

Leonardo

Edited by erpeo93

Share this post


Link to post
Share on other sites

I like the "accept one incoming connection at a time" API. You could even turn it into:

  ... inside top level loop ...
  while (NetworkConnection *conn = server->AcceptConnection()) {
    deal_with_new_connection(conn);
  }
  ...

You don't need to return the address as part of AcceptConnection(); you can just have a member on NetworkConnection.

 

I, too, think that specific channels aren't necessary at the lower level API. Some games will want to use object IDs, some games will want to use explicit channels, some games will want to use unstructured text, ... There's room for another API on top that does message sourcing/steering, as well as entity allocation/mirroring/despawning, and so on.

 

Yes, you need one copy when you compress/encrypt the payload, and one copy when you decompress/decrypt the payload. But you shouldn't copy more than that. You don't need an explicit "Release()" function, if you return an object instance (by value/copy/move) that is a smart reference that does the necessary refcounting. If you chase this direction down, you end up with multiple objects that point into different parts of the same buffer, and the buffer doesn't go away until all those objects have gone away. Then there's the question of the cost of the reference count, versus the cost of copying the data, which isn't a clear-cut win if you need to use atomic operations for reference counts. Which leads us to ...

 

Thread safety? It's OK to define an API that says "all functions must be called from the same thread" or at least "no two separate functions may be called at the same time from different threads." It's also OK to define some kind of more relaxed threading model, if that's convenient. If you want to go full-bore thread safe, copying data may again pop up as a reasonable thing to do -- only actual measurements will tell you for sure. But if you want lowest overhead, single-threaded API and non-atomic reference counts may win out.

 

Good luck on the library! You might want to look at other libraries like Enet and RakNet and Lidgren, and figure out specifically what you think you do better, and what you choose not to do for whatever reason, and create a little comparison matrix. That will make it easier for other people to understand what your goals are, and whether your library is right for them.

 

 

Share this post


Link to post
Share on other sites
14 hours ago, hplus0603 said:

...

 

I think that thread safety is one of the strength point of the library at the moment: you can send packets on the same connection from multiple threads, while receiving packets for the same connection on yet another thread.

 

I will have to think more about the "dispatch packet to application" part of the library, in the hope of finding the best possible solution in terms of both performance and readability of the api.

 

Thanks a lot for the feedback, it was exactly what I was searching for... I will definitively give you an update on this thread when the time comes. (Unfortunately I don't think I'll have time to work _solely_ on this library during the next months...) 

 

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  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!