CODEWITHSUNDEEP
delphiindy10
Atak_Snajpera
Atak_Snajpera

Reputation: 629

TIdCmdTCPServer and data synchronization with main thread [anomaly?]

Situation looks like this. External application client.exe sends a command MONITOR_ENCODING every ~250ms to the server. In server application I use IdCmdTCPServer1BeforeCommandHandler to read sent command from client. I copy received command (AData string) to global variable command and then show it in memo1. Meanwhile I have constantly running TSupervisorThread which copies global variable command to local Copiedcommand variable and then assigns new text to command variable. Is this an expected behaviour that from time to time instead of MONITOR_ENCODING text I get RESET?

enter image description here

procedure TForm1.IdCmdTCPServer1BeforeCommandHandler(ASender: TIdCmdTCPServer; var AData: String; AContext: TIdContext);
begin

  command:=AData;  //command is a global variable
  form1.Memo1.Lines.Add(IntToStr(form1.Memo1.Lines.Count+1)+'|'+IntToStr(GetTickCount)+'|'+command);

end;

procedure TSupervisorThread.CopyGlobalVariables;
begin

  CopiedCommand:=command; //Copiedcommand declared in TSupervisorThread
  command:='RESET'; 

end;

procedure TSupervisorThread.Execute;
begin

  while Terminated=false do
  begin
    Synchronize(CopyGlobalVariables);
    sleep(250);
  end;

end;

Upvotes: 0

Views: 158

Answers (1)

Remy Lebeau
Remy Lebeau

Reputation: 596256

FYI, the OnBeforeCommandHandler event is not the correct place to read commands with TIdCmdTCPServer. You are supposed to add an entry for each command to its CommandHandlers collection and then assign an OnCommand handler to each entry. The OnBeforeCommandHandler event is triggered before TIdCmdTCPServer parses a received command. That is OK for logging purposes, just make sure you don't use it to drive your processing logic. Leave that to the individual OnCommand events.

But, either way, command reading is done in a worker thread. You are not synchronizing with the main UI thread when adding received commands to your UI. You MUST synchronize. There are many ways to do that - TThread.Synchronize(), TThread.Queue(), TIdSync, TIdNotify, (Send|Post)Message(), etc, just to name a few.

More importantly, you have 2 threads (or more, depending on how many clients are connected at the same time) fighting over the same global variable without syncing access to it at all. You need a lock around the variable, such as a TCriticalSection or TMutex, or use Indy's TIdThreadSafeString class.

But, that won't solve the race condition that your code has. Between the time that your OnBeforeCommandHandler assigns a new value to command and the time that it reads command back to add it to the UI, your TSupervisorThread is free to modify command. That is exactly what you are seeing happen. It is not an anomaly in Indy, it is a timing issue in your code.

The easiest solution to that race condition is to simply add AData to your UI instead of command. That way, it won't matter if TSupervisorThread modifies command, your UI won't see it.

But, why are you using a global command variable at all? What is its real purpose? Are you trying to make an outside thread modify the commands that TIdCmdTCPServer parses? You are not controlling which clients get to parse real commands and which get fake commands. Why are you doing this?

Besides that, having TSupervisorThread perform 99% of its work in the main UI thread is a poor use of a worker thread, you may as well just use a TTimer in the UI instead. Otherwise, you need to coordinate your threads better, such as by using TEvent objects to signal when command is assigned to, and when it is reset.

I think you need to rethink your design.

Upvotes: 1

Related Questions