Delphi tIdTCPClient with timer events and other multi-threaded client side events

Question

We have a Delphi client server application using INDY. The client has a single tIdTCPClient connection to the server which is multi threaded. The client is "theoretically" a single thread. But in practice there are multiple threads on the client and this is where my problem is. For example think of a timer that fires every minute to get data from the server. And consider what happens when a user runs a command at the same time as this timer event. In truth, my problem is caused by our "Report Builder" reporting tool that (annoyingly) insists on loading every page of a report, which takes a while. The report runs off our "special" dataset that has a caching mechanism to transmit batches of records at a time (so multiple calls to the server to get all data). Meanwhile if a user does something else at the same time we seem to be getting crossed data. It seems the user getting back data that was meant for the report.

By the way, this bug is extremely rare, but a lot less rare for one particular customer who has the worlds slowest internet (luck me - I now have a test environment).

So on the client I have code a bit like this...

procedure DoCommand(MyIdTCPClient:tIdTCPClient; var DATA:tMemoryStream);
var
  Buffer: TBytes;
  DataSize: Integer;
  CommsVerTest: String;
begin
  //Write Data
  MyIdTCPClient.IOHandler.Write(DATA.Size);
  MyIdTCPClient.IOHandler.Write(RawToBytes(Data.Memory^,DataSize));

  //Read back 6 bytes CommsVerTest should always be the same (ie ABC123)
  SetLength(Buffer,0); //Clear out buffer
  MyIdTCPClient.IOHandler.ReadBytes(Buffer,6); 
  CommsVerTest:=BytesToString(Buffer);
  if CommsVerTest<>'ABC123' then
    raise exception.create('Invalid Comms');      //It bugs out here in rare cases

  //Get Result Data Back from Server
  DataSize:=MyIdTCPClient.IOHandler.ReadLongInt;   
  Data.SetSize(DataSize);                         //Report thread is stuck here
  MyIdTCPClient.IOHandler.ReadBytes(Buffer,DataSize);
end; 

Now when I debug it, I can confirm it bugs out when there are two threads in the middle of this procedure. The main thread stops at the exception. And the report thread is stuck somewhere else in the same procedure.

So, it looks to me like I need to make the procedure above thread safe. I mean so if the user wants to do something they just have to wait until the report thread finishes.

Arrrgh, I thought my client application was single threaded for sending data to the server!

I think that using TThread would not work - because I don't have access to the thread inside Report Builder. I think I need a tCriticalSection.

I think I need to make the application so that the above procedure can only be run by one thread at a time. Other threads have to wait.

Someone please help with the syntax.

Answer 1

TIdIOHandler has Write() and Read...() overloads for sending/receiving TStream data:

procedure Write(AStream: TStream; ASize: TIdStreamSize = 0; AWriteByteCount: Boolean = False); overload; virtual;

procedure ReadStream(AStream: TStream; AByteCount: TIdStreamSize = -1; AReadUntilDisconnect: Boolean = False); virtual;

You do not need to copy the TMemoryStream contents to an intermediate TIdBytes before sending it, or receive it as TIdBytes before copying it back to the TStream. In fact, there is nothing in in the code you have shown that needs to use TIdBytes directly at all:

procedure DoCommand(MyIdTCPClient: TIdTCPClient; var DATA: TMemoryStream);
var
  CommsVerTest: String;
begin
  //Write Data
  MyIdTCPClient.IOHandler.Write(DATA, 0, True);

  //Read back 6 bytes CommsVerTest should always be the same (ie ABC123)
  CommsVerTest := MyIdTCPClient.IOHandler.ReadString(6); 
  if CommsVerTest <> 'ABC123' then
    raise exception.create('Invalid Comms');

  //Get Result Data Back from Server
  DATA.Clear;
  MyIdTCPClient.IOHandler.ReadStream(DATA, -1, False);
end; 

With that said, if you have multiple threads writing to the same socket at the same time, or multiple threads reading from the same socket at the same time, they will corrupt each other's data (or worse). You need to synchronize access to the socket, such as with a critical section at a minimum. Because of your multi-threaded use of TIdTCPClient, you really need to re-think your overall client design.

At the very least, using your existing logic, when you need to send a command and read a response, stop the timer and wait for any pending data to be exchanged before then sending the command, and do not allow anything else to access the socket until the response comes back. You are trying to do too much at one time without synchronizing everything to avoid overlaps.

In the long run, it would be much safer to do all of the reading from a single dedicated thread and then pass any received data to your other threads for processing as needed. But that also means changing your sending logic to match. You could either:

1. If your protocol allows you to have multiple commands in flight in parallel, then you can send a command from any thread at any time (just be sure to use a critical section to avoid overlaps), but do not wait for a response immediately. Let each sending thread move on and do other things, and have the reading thread notify the appropriate sending thread asynchronously when the expected response actually arrives.

2. If the protocol does not allow for parallel commands, but you still need each sending thread to wait for its respective response, then give the socket thread a thread-safe queue that other threads can push commands into when needed. The socket thread can then run through that queue periodically sending each command and receiving its response one at a time as needed. Each thread that puts a command into the queue can include a TEvent to be signaled when the response arrives, that way they enter efficient sleep states while waiting, but you preserve your per-thread waiting logic.

Answer 2

Thanks Remy.

The TCriticalSection solved the problem. I have no control over things like the 3rd party report builder. And running reports entirely in their own thread wouldn't make much difference - they still need to share the same connection (I don't want or need parallel connections). Anyway the bulk of the program runs in the main thread, and it is rare that two threads need to communicate with the server at the same time.

So TCriticalSection was perfect - it prevented this procedure running twice at the same time (ie one thread has to wait until the first is finished). And happily - it worked brilliantly.

Basically the code now looks like this:

procedure DoCommand(
    CS:tCriticalSection; 
    MyIdTCPClient:tIdTCPClient; 
    var DATA:tMemoryStream);
var
  Buffer: TBytes;
  DataSize: Integer;
  CommsVerTest: String;
begin
  CS.Enter;     //enter Critical Section
  try
    //Write Data
    MyIdTCPClient.IOHandler.Write(DATA.Size);
    MyIdTCPClient.IOHandler.Write(RawToBytes(Data.Memory^,DataSize));

    //Read back 6 bytes CommsVerTest should always be the same (ie ABC123)
    SetLength(Buffer,0); //Clear out buffer
    MyIdTCPClient.IOHandler.ReadBytes(Buffer,6); 
    CommsVerTest:=BytesToString(Buffer);
    if CommsVerTest<>'ABC123' then
      raise exception.create('Invalid Comms');      

    //Get Result Data Back from Server
    DataSize:=MyIdTCPClient.IOHandler.ReadLongInt;   
    Data.SetSize(DataSize);                         
    MyIdTCPClient.IOHandler.ReadBytes(Buffer,DataSize);
  finally
    cs.Leave;
  end;
end;