Link to home
Start Free TrialLog in
Avatar of drama22
drama22

asked on

Thread safe opinion

i have client application i want to see how can i write it best way as thread safe , please if you found the code is miss help me to write it better !

here we go here is my client thread

type
  TClientThread = class(TThread)
  private
   FTCP: TIdTCPClient;
    procedure ProcessCommands(Command: string);
    procedure SendCommand(Command: string);
    procedure SendStream(Ms: TMemoryStream);
    procedure SendCommandAndStream(Command: String; Ms: TMemoryStream);
    procedure SendBuffer(Buffer: TIdBytes; BufferSize: Cardinal);
   //
  protected
  procedure Execute; override;

  Public
   constructor Create(CreateSuspended: Boolean; TCPClient: TIdTCPClient);
   destructor Destroy;
   procedure SendCommandWithParams(Command, Params: String);

  end;

Open in new window



Thread Create \ Destroy


constructor TClientThread.Create(CreateSuspended: Boolean;
  TCPClient: TIdTCPClient);
begin
inherited Create(CreateSuspended);
FreeOnTerminate := True;
FTCP := TCPClient.Create(nil);
FTCP.Host := TCPClient.Host;
FTCP.Port := TCPClient.Port;
FTCp.ConnectTimeout := TCPClient.ConnectTimeout;
FTCP.IPVersion := TCPClient.IPVersion;
FTCP.ReadTimeout := TCPClient.ReadTimeout;
FTCP.ReuseSocket := TCPClient.ReuseSocket;
FTCP.UseNagle := TCPClient.UseNagle;
end;

destructor TClientThread.Destroy;
begin
FTCP.Disconnect;
FTCP.Free;

end;

Open in new window



here is my client execute

procedure TClientThread.Execute;
var
Command : string;
begin
SendCommandWithParams('ENT', name + sep);
  while not Terminated do
    begin
        Command := FTCP.Socket.ReadLn;
      if Command <> '' then
      ProcessCommands(Command);
end;
end;

Open in new window



process command

procedure TClientThread.ProcessCommands(Command: string);
var
    Params: array [1 .. 10] of String;
    ParamsCount, P: Integer;
    PStr: String;
    IdBytes: TIdBytes;
    ReceiveParams, ReceiveStream: Boolean;
    Size: Int64;
  AudioDataSize: Cardinal;
  AudioData: Pointer;
    S : string;
  begin
 ReceiveParams := False;
  ReceiveStream := False;

if Command[1] = '1'  then //command with params
  begin
    Command := Copy(Command, 2, MaxInt);
    ReceiveParams := True;
  end
  else if Command[1] = '2' then //command + memorystream
  begin
    Command := Copy(Command, 2, MaxInt);
    ReceiveStream := True;
  end
  else if Command[1] = '3' then //command with params + memorystream
  begin
    Command := Copy(Command, 2, MaxInt);
    ReceiveParams := True;
    ReceiveStream := True;
  end;

  if ReceiveParams then //params incomming
  begin
    S := FTCP.Socket.ReadLn(IndyTextEncoding_UTF8);

    ParamsCount := 0;

while (S <> '') and (ParamsCount < 10) do
    begin
      Inc(ParamsCount);
      p := Pos(Sep, S);
      if p = 0 then
        Params[ParamsCount] := S
      else
      begin
        Params[ParamsCount] := Copy(S, 1, P - 1);
        Delete(S, 1, P + 4);
      end;
    end;
  end;

   MS := nil;
   try
    if ReceiveStream then //stream is incomming
    begin
      MS := TMemoryStream.Create;
      FTCP.Socket.LargeStream := True;
      FTCP.Socket.ReadStream(MS, -1, False);
      MS.Position := 0;
    end;




   Command := Trim(Command);


    if Command = 'SENDYOURINFO' then // succesfully loged in
    begin
      Form1.UniqueID := StrToInt(Params[1]);

      // enter room
        TThread.Synchronize(nil, Form1.updatecaption);
end else


  if Command = 'LEFT' then
  begin
  Form1.LineToadd :=  Params[2];
  Form1.LineTofilter :=  StripHTML(Params[1]);
  Form1.LineTocatch := Params[3];
  TThread.Synchronize(nil, Form1.endconnection);
  end else


if Command = 'AUDIO' then
  begin
    TThread.Synchronize(nil,procedure
  begin
      try
        EnterCriticalSection(Section);
        try
          Sleep(0);
          if (MS.Size > 10) and Form1.IsPlaying then
          begin
            try
              if not Form1.player.Active then
              begin
              Form1.player.Active := True;
              Form1.player.WaitForStart;
              end;
            except
            end;
            AudioDataSize := MS.Size;
            if BlockAlign > 1 then
              Dec(AudioDataSize, AudioDataSize mod BlockAlign);
            AudioData := AudioBuffer.BeginUpdate(AudioDataSize);
            try
              MS.ReadBuffer(AudioData^, AudioDataSize);
            finally
              AudioBuffer.EndUpdate;
            end;
          end
          else
          begin
            Form1.player.Active := False;
            Form1.player.WaitForStop;
          end;
        finally
          LeaveCriticalSection(Section);
        end;
      except
        // handle other exceptions here
      end;
      end);
    end;
   finally
     MS.Free;
   end;
end;

Open in new window

Avatar of Sinisa Vuk
Sinisa Vuk
Flag of Croatia image

Your procedure ProcessCommands contains lines where you call Form1. This is not thread safe at all.
This way it is better to make ProcessCommands synchronized:

type
  TClientThread = class(TThread)
  private
   FTCP: TIdTCPClient;
    procedure ProcessCommands(Command: string);
    procedure SendCommand(Command: string);
    procedure SendStream(Ms: TMemoryStream);
    procedure SendCommandAndStream(Command: String; Ms: TMemoryStream);
    procedure SendBuffer(Buffer: TIdBytes; BufferSize: Cardinal);
   //
  protected
  FCommand: String; //<---moved here
  procedure Execute; override;

  Public
   constructor Create(CreateSuspended: Boolean; TCPClient: TIdTCPClient);
   destructor Destroy;
   procedure SendCommandWithParams(Command, Params: String);

  end;
....

procedure TClientThread.Execute;
begin
SendCommandWithParams('ENT', name + sep);
FCommand := '';  //use class global variable
  while not Terminated do
    begin
        FCommand := FTCP.Socket.ReadLn;
      if FCommand <> '' then
        Synchronize(ProcessCommands);  //use sync
end;
end;

procedure TClientThread.ProcessCommands;  //modified to suit Synchronize
var
  Command: string;
....
begin
  Command := FCommand;  //new here...
....

  //so you can call Form1 as you do...

Open in new window

Avatar of drama22
drama22

ASKER

thank you very much sinisa , can you show me how to handle synchronize to call  procedures from form1 i been doing Thread,synchronize(nil, procedure to call from form1); after synchronizing process command iam thinking flat now also can you check my audio command ?
Avatar of drama22

ASKER

Also there is big problem when my client work very slow i don't know why
try to comment out line with synchonize. check if still all goes slow. or maybe problem is in process procedure.
Avatar of drama22

ASKER

speed issue is done my main issue i dont know how to synchronize procedures inside this process command for example my main issue in join and recive list here is my code with issue

when client start and send authentication to server i start to send commands like this inside form show for example

ClientThread.SendCommandWithParams('CLIENTJOINED', name + Sep );

then in process command handle i do several command that makes every thing complex
procedure TClientThread.ProcessCommands;

,,,,,,,,,,,,,,,,,
,,,,,,,,,,,,,

  if Command = 'CLIENTJOINED' then
  begin
  Form1.LineToJoinid := Params[2];;
  TThread.Synchronize(nil, Form1.Joindhandle);
  end else

 if Command = 'GETLIST' then
  begin

  TThread.Synchronize(nil, Form1.GETLISThandle);
  end
...........
end;

Open in new window


here is the procedures calls inside form1 each time new client joined
procedure TFORM1.Joindhandle;
begin

ClientThread.SendCommand('GETLIST');

end;

procedure usergetlist(Line: string; var strName);
var
  P, I: Integer;
begin
  I := 0;
  repeat
    P := Pos(Sep, Line);
    if P <> 0 then
    begin
      Inc(I);
      case I of
        1: strName     := Copy(Line, 1, P - 1);
         end;
      Delete(Line, 1, P + Length(Sep) - 1);
    end;
  until (I = 1) or (P = 0) or (Line = '')
end;


Procedure Tform1.Add_Item( strCaption: String);
var
userdataclass: Tuserdataclass;
begin

VDT1.BeginUpdate;
   try
   begin
userdataclass := Tuserdataclass.Create;
userdataclass.username:= strCaption;

  AddVSTStructure(VDT1,nil,userdataclass);

  finally
  VDT1.EndUpdate;
   end;
  end;


procedure Tform1.GETLISThandle;
var
I : integer;
Line: string;
strName : String;
begin
if Timer1.Enabled = true then
begin
Timer1.Enabled    := False;
end;
VDT1.BeginUpdate;
    try
    VDT1.Clear;
finally
    VDT1.EndUpdate;
  end;
    if Assigned(MS) then
    begin
      SL := TStringList.Create;
      try
        SL.LoadFromStream(MS);
        for I := 0 to SL.Count -1  do
        begin
            Line := SL.Strings[I];
            usergetlist(Line, strName);
           Add_Item(strName);
        end;
      finally
        SL.Free;
      end;
   end;

 Timer2.Interval := 80;
 Timer2.Enabled := True;

end;

Open in new window


the main issue is this list of virtuailtreestring updated very slow  some times names did not gone or there is maybe better way to populate the client list
also i dont know if TTHread,synchronize (nil, procedure ) is right way to sync
Avatar of drama22

ASKER

i still dont know how to write thread safe inside processcommand i feel TThread.synchronize(nil,,,,) is not the best also i still accsess FORM1 which is not safe hope to see full safe example code
ASKER CERTIFIED SOLUTION
Avatar of Sinisa Vuk
Sinisa Vuk
Flag of Croatia image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial