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

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

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...)