C# server - is this good way for timeouting and disconnecting?

Question

Hi, On my multithreaded server I am experiencincg troubles with connections that are not coming from the proper Client and so hang unathorized. I did not want to create new thread only for checking if clients are connected for some time without authorization. Instead of this, I have add this checking to RecieveData thread, shown on the code below. Do you see some performance issue or this is acceptable? The main point is that everytime client is connected (and Class client is instantionized) it starts stopwatch. And so I add to this thread condition - if the time is greater than 1 and the client is still not authorized, its added on the list of clients determinated for disconnection. Thanks

EDIT: This While(true) is RecieveData thread. I am using async. operations - from tcplistener.BeginAccept to threadpooling. I have updated the code to let you see more.

 protected void ReceiveData()
    {
        List<Client> ClientsToDisconnect = new List<Client>();
        List<System.Net.Sockets.Socket> sockets = new List<System.Net.Sockets.Socket>();
        bool noClients = false;
        while (true)
        {
            sockets.Clear();
            this.mClientsSynchronization.TryEnterReadLock(-1);
            try
            {
                for (int i = 0; i < this.mClientsValues.Count; i++)
                {
                    Client c = this.mClientsValues[i];
                    if (!c.IsDisconnected && !c.ReadingInProgress)
                    {
                        sockets.Add(c.Socket);
                    }
                    //clients connected more than 1 second without recieved name are suspect and should be disconnected
                     if (c.State == ClientState.NameNotReceived && c.watch.Elapsed.TotalSeconds > 1)
                         ClientsToDisconnect.Add(c);
                }
                if (sockets.Count == 0)
                {
                    continue;
                }
            }
            finally
            {

                this.mClientsSynchronization.ExitReadLock();
            }
            try
            {
                System.Net.Sockets.Socket.Select(sockets, null, null, RECEIVE_DATA_TIMEOUT);
                foreach (System.Net.Sockets.Socket s in sockets)
                {
                    Client c = this.mClients[s];
                    if (!c.SetReadingInProgress())
                    {
                        System.Threading.ThreadPool.QueueUserWorkItem(c.ReadData);

                    } 
                }
                //remove clients in ClientsToDisconnect
                foreach (Client c in ClientsToDisconnect)
                {
                    this.RemoveClient(c,true);
                }
            }
            catch (Exception e)
            {
                //this.OnExceptionCaught(this, new ExceptionCaughtEventArgs(e, "Exception when reading data."));
            }
        }
    }

Answer 1

My gut reaction is that while (true) plus if (sockets.Count == 0) continue will lead to heartache for your CPU. Why don't you put this on a Timer or something so that this function is only called every ~.5s? Is the 1s barrier really that important?

Answer 2

I think I see what you are trying to do and I think a better way would be to store new connections in a holding area until they have properly connected.

I'm not positive but it looks like your code could drop a valid connection. If a new connection is made after the checking section and the second section takes more than a second all the timers would time out before you could verify the connections. This would put the new connections in both the socket pool AND the ClientsToDisconnect pool. Not good. You would drop a currently active connection and chaos would ensue.

To avoid this, I would make the verification of a connection a seperate thread from the using of the connection. That way you won't get bogged down in timing issues (well...you still will but that is what happens when we work with threads and sockets) and you are sure that all the sockets you are using won't get closed by you.