TIdTCPServer sharing of serial port

Recently I changed a program which acts as a TCP server to help share the traffic on a serial port connected to a device. Multiple clients connect and should have access to the Serial Port and act simultaneously.

Application is built using C++Builder, using TIdTCPServer in the server and TIdTCPClient in the client application.

Multiple clients need to connect and send commands to the serial port. The serial port will respond immediately after sending a command to it, as per the protocols of the device it is attached to.

There is also a background thread which occasionally accesses the serial port and updates a memory cache of data held in the server's memory. The commands for sending and receiving from the serial port have a mutex on them, so they are accessible from both the TIdTcpServer's OnExecute event and the background thread.

I'm having difficulty getting the TIdTCPServer's OnExecute event to work without overlapping.

It would be really nice if the OnExecute event were to execute fully without another request coming in from another client, causing the overlapping.

Here is the OnExecute event handler of the TIdTCPServer:

void __fastcall TfrmMain::IServerExecute(TIdContext *AContext)
{
    int  i;
    int Len;
    TIdBytes TRB, TSB;
    unsigned char ARB[BUFFERLENGTH];
    int NumbSent, NumbReceived;

    // Read the command from the client.  Send the length first then the actual data.
    Len = AContext->Connection->Socket->ReadLongInt();
    AContext->Connection->Socket->ReadBytes(TRB, Len, false);

    memset(ARB,0,BUFFERLENGTH);
    for(i=0;i<Len;i++) AOB[i]=TRB[i];

    NumbSent=Len;


    // Now send it out to the Serial port
    ProcessSerialMessage(AOB, Len, ARB, &NumbReceived, false);

    sending=false;

    TSB.Length=NumbReceived;
    for(i=0;i<TSB.Length;i++) TSB[i]=ARB[i];
    AContext->Connection->Socket->Write(TSB.Length);
    AContext->Connection->Socket->Write(TSB);

    return;
}

Here is the routine for sending the data out over the serial port:

int ProcessSerialMessage(unsigned char *SendBuf, int NumbSBytes, unsigned char *ReceiveBuf, int *NumbRBytes, bool CalledFromThread)
{

    // MMUtex is a global TMutex Object
    // Mutex required to help with the background thread trying to update memory cache.
    MMutex->Acquire();


    // Ok now send the data out over the serial port and receive it.
    // These routines are standard serial port I/O routines and aren't explained here.
    rawsend(SendBuf, NumbSBytes);
    rawreceive(ReceiveBuf, NumbRBytes);

    RetValue=*NumbRBytes;
    MMutex->Release();

    return(RetValue);
}

Solution

TIdTCPServer is a multi-threaded component. Each connected client runs in its own independent thread. The OnExecute event runs in those threads. So, it is your responsibility to make sure your OnExecute code is thread-safe, by serializing access to any shared resources.

You are using a mutex inside of ProcessSerialMessage(), so you are serializing access to the serial port (assuming your other background thread is also entering the same mutex). So that should be fine (although, I would suggest protecting the mutex locking/unlocking using a try..__finally block, or a local RAII-style class, to ensure the mutex is unlocked properly even if an exception is thrown).

However, one major issue I see with this code is that your AOB (and sending) variable is not declared as a local variable to IServerExecute(), which means it must be a shared variable accessed across threads (UPDATE: you have confirmed that in comments: "[AOB is] declared globally."). But, it is not being protected from concurrent access by multiple threads, which means that multiple clients are free to overwrite each other's inbound data while it is being sent to the serial port.

You are reading the serial port's response into local variables, and then using them to send back to the requesting clients. So there is no concurrency issue in that code.

I would suggest passing your TRB and TSB arrays directly to ProcessSerialMessage(). The 2 loops you have to copy bytes from one array to another are not really necessary, thus you can eliminate the AOB and ARB variables from this code completely. That might be enough to solve your issue.

Try this:

void __fastcall TfrmMain::IServerExecute(TIdContext *AContext)
{
    TIdBytes TRB, TSB;
    int NumbSent, NumbReceived;

    // Read the command from the client.  Send the length first then the actual data.
    NumbSent = AContext->Connection->Socket->ReadLongInt();
    AContext->Connection->Socket->ReadBytes(TRB, NumbSent, false);

    TSB.Length = BUFFERLENGTH;

    // Now send it out to the Serial port
    ProcessSerialMessage(&TRB[0], NumbSent, &TSB[0], &NumbReceived, false);

    AContext->Connection->Socket->Write(NumbReceived);
    AContext->Connection->Socket->Write(TSB, NumbReceived);
}