#32 Issue handling multiple clients

[wizardbrony]

Symptom:

• Client1: Connect
• Client2: Connect
• Client1: Disconnect

The server will no longer respond to Client2.

Cause:

The server logic uses an array called m_clients (with a length of MAX_CLIENTS) to persist active socket connections. The value at each index of this array may be either a valid Socket or INVALID_SOCKET. This implies that there may be "holes" in the array (in other words, INVALID_SOCKETs between active Sockets). To deal with these holes, the server uses a simple pattern to only process valid Sockets:

for(int i=0;i<MAX_CLIENTS;i++) {
    if(m_clients[i]!=INVALID_SOCKET) {
        ...

The method ThreadProc also has a separate array called events that contains a separate CEvent for each object that is able to trigger events. This array is then passed to the WaitForMultipleObjects function to wait until one of the event objects becomes signaled:

        int count=0;
        events[count++] = m_serverThreadEvent;  // exit event
        events[count++] = serverEvent;

        for(i=0;i<MAX_CLIENTS;i++) {
            if(m_clients[i]!=INVALID_SOCKET) {
                events[count++] = clientEvent[i];
            }
        }

        DWORD res = WaitForMultipleObjects(count,events,FALSE,INFINITE);

Note the use of the pattern above to only include the CEvents for valid Sockets. This effectively removes any holes from events that still exist in m_clients. As a result, if holes do exist in m_clients, when a client network event occurs the value returned from WaitForMultipleObjects (res) will not align with the indices of m_clients.

With that in mind, here is the start of the logic for handling a client network event:

        else /* client closed or data received */
        {
            for(i=0;i<MAX_CLIENTS;i++)
            {
                if(m_clients[i]!=INVALID_SOCKET)
                {
                    if(res==(WAIT_OBJECT_0+(2+i)))
                    {
                        ...

The problem is that res is being compared to a value based on i, which is an index of m_clients. This is why the symptom scenario occurs: Disconnecting Client1 causes a hole in m_clients, so when a Client2 network event occurs, res will not be aligned with i.

Solution:

One solution is a simple map:

    int     waitObjectOffsets[MAX_CLIENTS];

...

        for(i=0;i<MAX_CLIENTS;i++) {
            if(m_clients[i]!=INVALID_SOCKET) {
                waitObjectOffsets[i] = count;
                events[count++] = clientEvent[i];
            }
        }

...

        else /* client closed or data received */
        {
            for(i=0;i<MAX_CLIENTS;i++)
            {
                if(m_clients[i]!=INVALID_SOCKET)
                {
                    if(res==(WAIT_OBJECT_0+waitObjectOffsets[i]))
                    {

waitObjectOffsets maps an index of m_clients back to the associated index of events. This allows the server to correctly handle client events even when m_clients has holes.