Why can I only connect one client to my server?

Question

I am currently working on a Udp server and client system, where I worked out a way to "verify" which user is sending the updates. Essentially, the very first package they send is "blank", which the server takes in, increments a list by 1, and then does a Send() to the port, which I believe should go to all connected Clients (please correct me if I'm wrong on this assumption). The client will ignore any new "ID's" that are coming in after theirs is assigned, and the ID part of the server will ignore any packages that don't start with a blank space.

My current issue is:

I can't seem to get more than one client connected, and I'm a little stumped on why.

Server/Client Manager:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System.Threading;

public class ServerManager : MonoBehaviour
{
    private GameObject serverGO;
    private Thread serverThread;
    private Thread clientThread;
    private Server1 serverScript;
    private Client1 clientScript;
    private bool serverThreadRunning = false;
    private bool clientThreadRunning = false;

    // Start is called before the first frame update
    void Start()
    {
        serverGO = this.gameObject;
        serverScript = new Server1();
        clientScript = new Client1();
        serverThread = new Thread(serverScript.Listen);
        clientThread = new Thread(clientScript.SendData);
    }

    // Update is called once per frame
    void Update()
    {
        Control();

        Debug.Log(serverScript.GetClientID().Count);
        Debug.Log(serverScript.GetContact());
    }

    private void OnApplicationQuit()
    {
        serverScript.done = true;
    }

    private void Control()
    {
        if (Input.GetKeyDown(KeyCode.P) && !(serverThreadRunning) && !(clientThreadRunning))
        {
            serverThreadRunning = true;
            serverThread.Start();
        }

        else if (Input.GetKeyDown(KeyCode.L) && !(serverThreadRunning) && !(clientThreadRunning))
        {
            clientThreadRunning = true;
            clientThread.Start();
        }

        if (Input.GetKeyDown(KeyCode.M) && clientThreadRunning)
        {
            clientScript.disconnect = true;
        }
    }
}

Server:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;

public class Server1 : MonoBehaviour
{
    private CancellationTokenSource cancellationSource = new CancellationTokenSource();
    Dictionary<int, IPEndPoint> clientID = new Dictionary<int, IPEndPoint>();
    private int done = 0;
    public string rds;


    public void Listen()
    {
        int port = 8080;
        Socket serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        IPEndPoint ep = new IPEndPoint(IPAddress.Any, port);
        UdpClient listener = new UdpClient(ep);
        byte[] rData;
        string rDataString;
        byte[] sendData;
        string sendDataString;
        var token = cancellationSource.Token;

        try
        {
            while (token.IsCancellationRequested)
            {
                var receiveTask = listener.ReceiveAsync();
                var result = receiveTask.Result;
                rData = result.Buffer;
                rDataString = Encoding.ASCII.GetString(rData);

                if (rDataString.Equals(" "))
                {
                    var clientAddress = ep.Address;
                    ep = new IPEndPoint(clientAddress, port);
                    var id = (clientID.Count + 1);
                    sendDataString = id.ToString();
                    sendData = Encoding.ASCII.GetBytes(sendDataString);
                    clientID.Add(id, ep);
                    var clientEP = result.RemoteEndPoint;
                    serverSocket.SendTo(sendData, ep);
                    //serverSocket.Close();
                }

                else
                {
                    rds = rDataString;
                }
            }
        }

        catch (Exception e)
        {
            Debug.Log("Server error: " + e.ToString());
        }

        listener.Close();
    }

    public Dictionary<int, IPEndPoint> GetClientID()
    {
        return clientID;
    }

    public string GetContact()
    {
        return rds;
    }

    public void Stop()
    {
        cancellationSource.Cancel();
    }
}

Client:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System.Net;
using System.Net.Sockets;
using System;
using System.Text;

public class Client1 : MonoBehaviour
{
    private GameObject player;
    private Transform playerPos;

    public bool disconnect = false;
    public bool requestClientId = false;

    public void SendData()
    {
        Move updatePlayer = new Move();
        GameObject player;
        Transform playerPos;
        Client1 parent = new Client1();
        Socket clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        int port = 8080;
        IPAddress ip = IPAddress.Parse("10.10.33.24");
        IPEndPoint ipep = new IPEndPoint(ip, port);
        bool disconnect = false;
        byte[] sendData;
        int clientID = -1;
        byte[] rData;
        string rDataString;

        while (true)
        {
            if (parent.disconnect != disconnect)
            {
                disconnect = parent.disconnect;
            }

            try
            {
                if (clientID < 0)
                {
                    sendData = Encoding.ASCII.GetBytes(" ");
                    clientSocket.SendTo(sendData, ipep);

                    UdpClient listener = new UdpClient(ipep);
                    rData = listener.Receive(ref ipep);
                    rDataString = Encoding.ASCII.GetString(rData);

                    clientID = Int32.Parse(rDataString);

                    listener.Close();
                }

                else if (clientID > 0)
                {
                    sendData = Encoding.ASCII.GetBytes("didit");
                    clientSocket.SendTo(sendData, ipep);
                }
            }

            catch (Exception e)
            {

            }

            if (disconnect)
            {
                clientSocket.Close();
            }
        }
    }
}

This is clearly an unfinished product, I am aware I'm missing some key future pieces (removing players from list, having the designated time for the server to do a listener.Close()), but for now I'm just trying to get multiple clients connected at the same time, and am looking for a point in the right direction.

---

Edit:

Updated Server code

Answer 1

---

Update

These are the lessons learned from fixing this problem:

We found the following issues when reviewing the network code:

1. [Client/Server] UdpClient.Receive alters the IPEndPoint you pass to it by reference. So, do not use the same endpoint you use for sending.

2. [Server] We needed to use ReceiveAsync on the server to be able to interrupt it. Which we do with a CancellationToken. However, ReceiveAsync does not take a CancellationToken, thus we need to take the task it returns and call Wait with the token, then take the result of the task. Which also makes it pointless to use async/await, since we should not await the task to be able to pass the CancellationToken. which also means the server networking code must run on its own thread, we already knew that part.

3. [Server] Since we are using a CancellationToken, we will make a loop while(!token.IsCancellationRequested) on the server thread. It was an easy mistake to make to forget that !, my bad.

4. [Client/Server] Speaking of simple mistakes. Having Debug.Log calls helped indentify some problems. You should have it in your catch. Debug.Log also helped figure out...

5. [Client] We should not create an UdpClient using the server endpoint※, or any endpoint that specifies a port for that matter. That would make it listen on that port, and we cannot have multiple clients on the same machine listening on the same port at the same time. ※: With the server and the client running on the same server, this was preventing the client from receiving.

6. [Client] Instead we should create an UdpClient and set the socket, something like this: var listener = new UdpClient { Client = clientSocket }. And while we are at it, why not use a using statement?

7. [Client] Turns out that disposing the UdpClient closes the socket. Thanks Debug.Log. Thus, do not create an UdpClient every time you want to listen, just create the socket and the UdpClient once before the loop. In fact, put the loop inside the using statement.

Bonus:

• You can restrict from where the UdpClient.Receive will receive by setting the IPEndPoint you pass to it. Which it will modify. Thus, on the client, create a new IPEndPoint to pass to Receive, make it equal to the one used to send to the server. But do not use the same IPEndPoint, becase Receive modifies it, and then sending does not work. Did I mention that Receive modifies the endpoint? The server does not have that problem by virtue of using RecieveAsync, which arguebly isn't a bad idea to use on the client too.

• It could be a good idea to have a Dictionary<int, IPEndPoint> to store the IPEndPoint of each client on the server. So, when the server needs to send to a particular client, it look up there by id. Similarly, it could be a good idea to have a Dictionary<IPEndPoint, int> so the server can recognize the client by its endpoint, and thus know which one is sending.

Note: We are not setting the ip and port of the client (by not creating the UdpClient with the IPEndPoint but setting the socket instead). We are only setting the ip and port of the server. The ip of the client is whatever it is, and the port is whatever the system assigns to it. That means that when you have multiple clients on the same machine, they end up with different ports, and thus they can all listen at the same time.

---

Addendum on what does Receive actually do

This the reference code for UdpClient.Receive (I replaced the comments with my own):

public byte[] Receive(ref IPEndPoint remoteEP) {
    if (m_CleanedUp){
        throw new ObjectDisposedException(this.GetType().FullName);
    }

    EndPoint tempRemoteEP;

    if ( m_Family == AddressFamily.InterNetwork ) {
        tempRemoteEP = IPEndPoint.Any;
    }
    else {
        tempRemoteEP = IPEndPoint.IPv6Any;
    }

    int received = Client.ReceiveFrom(m_Buffer, MaxUDPSize, 0 , ref tempRemoteEP);
    remoteEP = (IPEndPoint)tempRemoteEP; // <- removeEP is set, never read

    if (received < MaxUDPSize) {
        byte[] newBuffer = new byte[received];
        Buffer.BlockCopy(m_Buffer,0,newBuffer,0,received);
        return newBuffer;
    }
    return m_Buffer;
}

We can observe that from where the UdpClient will receive depends on how it was set up, not the passed IPEndPoint. In fact the passed IPEndPoint is set, never read. If we look at UdpClient.Receive in .NET Core we can see the same remains true. I have also confirmed with Telerik JustDecompile. Therefore IPEndPoint remoteEP should have been an out parameter, not ref.

---

Old answer

1. Receive sets ep (passed as ref) to the end point at the other side, the one that sent the data.

2. SendTo sends to the endpoint provided:

   serverSocket.SendTo(sendData, ep);
   

---

Your code does not multi-cast. And I would argue you should not be trying to multi-cast anyway.

The server can build a dictionary of client ids and end points. Then it can send to the right client.

Something like this:

Dictionary<int, IPEndPoint> clients = new Dictionary<int, IPEndPoint>();

---

Which remind me, Receive blocks the thread. Remember that when writing the code. I will also remind you that there is a ReceiveAsync, which despite the name was not created to work with async/await (hence the lack of CancellationToken on the call, you are supposed to call Wait(cancellationToken) on the task it returns).

---

Addendum on threading and cancellation

Currently the server class has a field public bool done = false;. For encapsulation, let us make that private and add a Stop method.

private bool done = false;

public void Stop()
{
    done = true;
}

And, of course, on the server manager, we will change

serverScript.done = true;

to

serverScript.Stop();

This is better, bot not good. However, this change is necessary because we will start messing with it.

The problem is that the value of the bool done might never be visible to other thread. We can improve that situation by using a volatile.

private volatile bool done = false;

That should work, at least better. It does not work well in some old and buggy platforms. Hopefully none of those is out there, regardless I am paranoid on threading, so I recommend using Thread.VolatileRead and Thread.VolatileWrite or Volatile.Read and Volatile.Write depending on the platform.

Would look something like this:

private int done = 0;

public void Stop()
{
    Volatile.Write(ref done, 1);
}

And the check:

while (Volatile.Read(ref done) == 0)
{
    // ...
}

And we are not there yet. Why? Receive blocks the thread Which means that the thread would be waiting on the data from the client, and thus would not notice that done changed...

Alright, so we neeed a CancellationToken. The idea is that we will create a CancellationTokenSource:

private CancellationTokenSource cancellationSource = new CancellationTokenSource();

public void Stop()
{
    cancellationSource.Cancel();
}

Then the check looks like this:

var token = cancellationSource.Token;
while (!token.IsCancellationRequested)
{
    // ...
}

Note: We want to loop while the cancellation has not been requested.

We are taking the token, we are going to need it for Receive... or rather ReceiveAsync:

var receiveTask = listener.ReceiveAsync();

// The following call can be interrupted by the token
// It will throw `OperationCanceledException` if that happens, catch it
receiveTask.Wait(token);
var result = receiveTask.Result;

// This is the data from the client
rData = result.Buffer; 

// ... you can check for " " here ...

// Store the following endpoint in the client dictionary
// and use it to send data to the client
var clientEP = result.RemoteEndPoint;

Note: This API was not designed to be used with async/await, so do not try to convert the code to use async/await.

---

I'd expect the code to look like this:

var id = (clientID.Count + 1);
sendDataString = id.ToString();
sendData = Encoding.ASCII.GetBytes(sendDataString);
var clientEP = result.RemoteEndPoint;
clientID.Add(id, clientEP);
serverSocket.SendTo(sendData, clientEP);

---

Addendum on BeginReceive

If ReceiveAsync is not working, we have another option in BeginReceive. The following is adapted from the MSDN example:

private int done;
private Dictionary<int, IPEndPoint> clientID = new Dictionary<int, IPEndPoint>();
private Socket serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
// public string rds;

private void Handle(IPEndPoint clientEP, string receiveString)
{
    if (receiveString.Equals(" "))
    {
        var id = (clientID.Count + 1);
        sendDataString = id.ToString();
        sendData = Encoding.ASCII.GetBytes(sendDataString);
        clientID.Add(id, clientEP);
        serverSocket.SendTo(sendData, clientEP);
    }
    else
    {
        // rds = rDataString;
    }
}

private void ReceiveCallback(IAsyncResult ar)
{
    if (Thread.VolatileRead(ref done) == 1)
    {
        return;
    }

    var listener = ((UdpClient)(ar.AsyncState));
    IPEndPoint clientEP;

    byte[] receiveBytes = listener.EndReceive(ar, ref clientEP);
    string receiveString = Encoding.ASCII.GetString(receiveBytes);

    Handle(clientEP, receiveString);

    // When a message comes, it will call ReceiveCallback
    listener.BeginReceive(new AsyncCallback(ReceiveCallback), listener);
}

public void Listen()
{
    if (Thread.VolatileRead(ref done) == 1)
    {
        return;
    }

    int port = 8080;
    var serverEP = new IPEndPoint(IPAddress.Any, port);
    var listener = new UdpClient(serverEP);

    // When a message comes, it will call ReceiveCallback
    listener.BeginReceive(new AsyncCallback(ReceiveCallback), listener);
}

public void Stop()
{
    Volatile.Write(ref done, 1);
}

We should be able to incorporate the CancellationToken using TaskFactory.FromAsync, however I would prefer to use ReceiveAsync instead.