TCP server connection causing processor overload

Question

I have a TCP/IP server that is supposed to allow a connection to remain open as messages are sent across it. However, it seems that some clients open a new connection for each message, which causes the CPU usage to max out. I tried to fix this by adding a time-out but still seem to have the problem occasionally. I suspect that my solution was not the best choice, but I'm not sure what would be.

Below is my basic code with logging, error handling and processing removed.

private void StartListening()
{            
    try
    {
        _tcpListener = new TcpListener( IPAddress.Any, _settings.Port );
        _tcpListener.Start();
        while (DeviceState == State.Running)
        {
            var incomingConnection = _tcpListener.AcceptTcpClient();
            var processThread = new Thread( ReceiveMessage );
            processThread.Start( incomingConnection );
        }
    }
    catch (Exception e)
    {
        //  Unfortunately, a SocketException is expected when stopping AcceptTcpClient
        if (DeviceState == State.Running) { throw; }
    }
    finally { _tcpListener?.Stop(); }
}

I believe the actual issue is that multiple process threads are being created, but are not being closed. Below is the code for ReceiveMessage.

    private void ReceiveMessage( object IncomingConnection )
    {
        var buffer = new byte[_settings.BufferSize];
        int bytesReceived = 0;
        var messageData = String.Empty;
        bool isConnected = true;

        using (TcpClient connection = (TcpClient)IncomingConnection)
        using (NetworkStream netStream = connection.GetStream())
        {
            netStream.ReadTimeout = 1000;
            try
            {
                while (DeviceState == State.Running && isConnected)
                {
                    //  An IOException will be thrown and captured if no message comes in each second. This is the
                    //  only way to send a signal to close the connection when shutting down. The exception is caught,
                    //  and the connection is checked to confirm that it is still open. If it is, and the Router has
                    //  not been shut down, the server will continue listening.
                    try { bytesReceived = netStream.Read( buffer, 0, buffer.Length ); }
                    catch (IOException e)
                    {
                        if (e.InnerException is SocketException se && se.SocketErrorCode == SocketError.TimedOut)
                        {
                            bytesReceived = 0;
                            if(GlobalSettings.IsLeaveConnectionOpen)
                                isConnected = GetConnectionState(connection);
                            else
                                isConnected = false;
                        }
                        else
                            throw;
                    }

                    if (bytesReceived > 0)
                    {
                        messageData += Encoding.UTF8.GetString( buffer, 0, bytesReceived );
                        string ack = ProcessMessage( messageData );
                        var writeBuffer = Encoding.UTF8.GetBytes( ack );
                        if (netStream.CanWrite) { netStream.Write( writeBuffer, 0, writeBuffer.Length ); }
                        messageData = String.Empty;
                    }
                }
            }
            catch (Exception e) { ... }
            finally { FileLogger.Log( "Closing the message stream.", Verbose.Debug, DeviceName ); }
        }
    }

For most clients the code is running correctly, but there are a few that seem to create a new connection for each message. I suspect that the issue lies around how I handle the IOException. For the systems that fail, the code does not seem to reach the finally statement until 30 seconds after the first message comes in, and each message creates a new ReceiveMessage thread. So the logs will show messages coming in, and 30 seconds in it will start to show multiple messages about the message stream being closed.

Below is how I check the connection, in case this is important.

public static bool GetConnectionState( TcpClient tcpClient )
{
    var state = IPGlobalProperties.GetIPGlobalProperties()
        .GetActiveTcpConnections()
        .FirstOrDefault( x => x.LocalEndPoint.Equals( tcpClient.Client.LocalEndPoint )
        && x.RemoteEndPoint.Equals( tcpClient.Client.RemoteEndPoint ) );
    return state != null ? state.State == TcpState.Established : false;
}

Answer 1

You're reinventing the wheel (in a worse way) at quite a few levels:

1. You're doing pseudo-blocking sockets. That combined with creating a whole new thread for every connection in an OS like Linux which doesn't have real threads can get expensive fast. Instead you should create a pure blocking socket with no read timeout (-1) and just listen on it. Unlike UDP, TCP will catch the connection being terminated by the client without you needing to poll for it.

2. And the reason why you seem to be doing the above is that you reinvent the standard Keep-Alive TCP mechanism. It's already written and works efficiently, simply use it. And as a bonus, the standard Keep-Alive mechanism is on the client side, not the server side, so even less processing for you.

Edit: And 3. You really need to cache the threads you so painstakingly created. The system thread pool won't suffice if you have that many long-term connections with a single socket communication per thread, but you can build your own expandable thread pool. You can also share multiple sockets on one thread using select, but that's going to change your logic quite a bit.