TIdServer, about synchronization again

Question

According to several ask and almost satysfy answers a specialy by Remy Lebeau (thank you again) I try to combine code usefull for my application. And several aspects leave for me unclear. When you look at code below:

• when I use Button3Click procedure to send BroadCast from GUI to connected clients - is it correct method (I mean: is it safe)?
• can I put in similar to DoSomethingSafe method code in which I create connection to DB, do something on it, and close connection to DB? Is it safe?
• why my application freezes when there are more then 20 clietns and I want use to stop working server by usingh its active:=false (button2.click method)?
• can I use TCliContext.BroadcastMessage inside the TCliContext.ProccessMsg just invoking it wihtout any synchronization?
• can I read (Connection.IOHandler.ReadLn()) in OnConnect method (I want read line with login data and check it in DB then when it is incorrect do disconnection immediatly?
• I read somewhere, using IdSync is sometimes hazard (if anything goes wrong inside using it), therefore I have last question: what is better solution to reach global variables or VCL objects?

My exemplary code looks like:

type
  TCliContext = class(TIdServerContext)
  private
    Who: String;
    Queue: TIdThreadSafeStringList;

    Activity_time: TDateTime;
    Heartbeat_time: TDateTime;

    InnerMessage: String;

    procedure BroadcastMessage(const ABuffer: String);
    procedure SendMessageTo(const ADestUser: String; const ABuffer: String);

  public
    constructor Create(AConnection: TIdTCPConnection; AYarn: TIdYarn; AList: TThreadList = nil); override;
    destructor Destroy; override;

    procedure ProccessMsg;
    procedure DoSomethingSafe;
    procedure info_about_start_connection;
  end;

procedure TCliContext.BroadcastMessage(const ABuffer: String);
var
  cList: TList;
  Count: Integer;
  CliContext: TCliContext;
begin
  cList := Server.Contexts.LockList;
  try
    for Count := 0 to cList.Count - 1 do
    begin
      CliContext := TCliContext(cList[Count]);
      if CliContext <> Self then
        CliContext.Queue.Add(ABuffer);
    end;
  finally
    Server.Contexts.UnlockList;
  end;
end;

procedure TCliContext.SendMessageTo(const ADestUser: String;
  const ABuffer: String);
var
  cList: TList;
  Count: Integer;
  CliContext: TCliContext;
begin
  cList := Server.Contexts.LockList;
  try
    for Count := 0 to cList.Count - 1 do
    begin
      CliContext := TCliContext(cList[Count]);
      if CliContext.Who = ADestUser then
      begin
        CliContext.Queue.Add(ABuffer);
        Break;
      end;
    end;
  finally
    Server.Contexts.UnlockList;
  end;
end;

constructor TCliContext.Create(AConnection: TIdTCPConnection; AYarn: TIdYarn; AList: TThreadList = nil);
begin
  // inherited Create(AConnection, AYarn, AList);
  inherited;
  Queue := TIdThreadSafeStringList.Create;
end;

destructor TCliContext.Destroy;
begin
  Queue.Free;
  inherited;
end;

procedure TCliContext.ProccessMsg;
begin
  InnerMessage := Connection.IOHandler.ReadLn();
  TIdSync.SynchronizeMethod(DoSomethingSafe);
  // is it ok?
end;

procedure TCliContext.info_about_start_connection;
begin
  MainForm.Memo1.Lines.Add('connected');
end;

procedure TCliContext.DoSomethingSafe;
begin
  MainForm.Memo1.Lines.Add(InnerMessage);
end;

and code corelate with GUI

procedure TMainForm.BroadcastMessage(Message: string);
var
  cList: TList;
  Count: Integer;
begin
  cList := IdTCPServer.Contexts.LockList;
  try
    for Count := 0 to cList.Count - 1 do
      TCliContext(cList[Count]).Queue.Add(Message);
  finally
    IdTCPServer.Contexts.UnlockList;
  end;
end;

procedure TMainForm.FormCreate(Sender: TObject);
begin
  IdTCPServer.ContextClass := TCliContext;
end;

procedure TMainForm.IdTCPServerConnect(AContext: TIdContext);
begin
  TCliContext(AContext).Queue.Clear;
  TCliContext(AContext).Heartbeat_time := now;
  TCliContext(AContext).Activity_time := now;
  TIdSync.SynchronizeMethod(TCliContext(AContext).info_about_start_connection);
  // is it safe?
end;

procedure TMainForm.IdTCPServerExecute(AContext: TIdContext);
var
  tmplist, Queue: TStringlist;
  dtNow: TDateTime;
begin
  dtNow := now;
  tmplist := nil;
  try
    Queue := TCliContext(AContext).Queue.Lock;
    try
      if Queue.Count > 0 then
      begin
        tmplist := TStringlist.Create;
        tmplist.Assign(Queue);
        Queue.Clear;
      end;
    finally
      TCliContext(AContext).Queue.Unlock;
    end;
    if tmplist <> nil then
    begin
      AContext.Connection.IOHandler.Write(tmplist);
      TCliContext(AContext).Heartbeat_time := dtNow;
    end;
  finally
    tmplist.Free;
  end;

  if SecondsBetween(dtNow, TCliContext(AContext).Heartbeat_time) > 30 then
  begin
    AContext.Connection.IOHandler.WriteLn('E:');
    TCliContext(AContext).Heartbeat_time := dtNow;
  end;

  if SecondsBetween(dtNow, TCliContext(AContext).Activity_time) > 6 then
  begin
    AContext.Connection.Disconnect;
    Exit;
  end;
  TCliContext(AContext).ProccessMsg;;
end;

procedure TMainForm.Button1Click(Sender: TObject);
begin
  IdTCPServer.Active := true;
end;

procedure TMainForm.Button2Click(Sender: TObject);
begin
  IdTCPServer.Active := false;
  // here application freezes when there are more then tens active clients
end;

procedure TMainForm.Button3Click(Sender: TObject);
begin
  BroadcastMessage('Hello');
  // is it safe and correct?
end;

Update (The last question after your excellent answer) To do simpler (shorter length of code) can I use TIdNotify class like below:

TMyNotify.Create(1, 'ABC').Notify; 

.

type
  TMyNotify = class(TidNotify)
  public
    faction: string;
    fdata:string;
    procedure DoNotify; override;
    procedure action1();
    procedure action2();
    constructor Create(action:integer;fdata:string); reintroduce;
  end;

constructor TMyNotify.Create(action:integer;fdata:string); reintroduce;
begin
  inherited Create;
  faction:=action;
  fdata:=data;
end;

procedure TMyNotify.action2()
begin
  //use fdata and do something with vcl etc.
end;

procedure TMyNotify.action2()
begin
  //use fdata and do something with vcl etc.
end;

procedure TMyNotify.DoNotify;
begin
  case action of
    1: action1()
    2: action2()
  end;
end;

Thank you again for previous help

Answer 1

when I use Button3Click procedure to send BroadCast from GUI to connected clients - is it correct method (I mean: is it safe)?

Yes, you are sending the data correctly and safely. However, TCliContext.ProcessMsg() is performing a blocking call to ReadLn(). If the client is not sending any data for awhile, that logic prevents your OnExecute code from performing its time-sensitive logic in a timely manner, if at all. Since you have time-sensitive logic involved, you need to use timeouts on your connection handling so your time checks have oppurtunity to run. Do not call ProcessMsg() until there is actual data available to read (or have ProcessMsg() handle the timeouts internally), and you should assign a value to the TIdIOHandler.ReadTimeout property for good measure in case the client stops sending data in the middle of a message. For example:

procedure TMainForm.IdTCPServerConnect(AContext: TIdContext);
begin
  ...
  AContext.Connection.IOHandler.ReadTimeout := 10000;
end;

procedure TMainForm.IdTCPServerExecute(AContext: TIdContext);
var
  ...
begin
  ...

  if AContext.Connection.IOHandler.InputBufferIsEmpty then
  begin
    if not AContext.Connection.IOHandler.CheckForDataOnSource(100) then
    begin
      AContext.Connection.IOHandler.CheckForDisconnect;
      Exit;
    end;
  end;

  TCliContext(AContext).ProccessMsg;
  TCliContext(AContext).Activity_time := Now();
end;

can I put in similar to DoSomethingSafe method code in which I create connection to DB, do something on it, and close connection to DB? Is it safe?

Yes. In fact, especially for DB queries, you should give each client thread its own connection to the DB whenever possible. Then you don't have to sync the DB queries (and depending on the DB used, you might even be able to use DB-provided synchronization locks in the queries themselves). You should also pool the DB connections, if possible (some DB types are not poolable due to architectual restrictions. ADO, for example, due to its use of thread-specific ActiveX/COM objects). Do not sync a DB connection across multiple threads if you do not have to. When you need to perform a DB query, obtain a DB connection from the pool (or create a new connection if needed), do the DB query, and then put the DB connection back in the pool (if possible) so another client thread can use it when needed. If a DB connection is in the pool for awhile, disconnect it and then reconnect it when it needs to be used again. This helps keep the number of DB connections to a minimum while maximizing their usage.

why my application freezes when there are more then 20 clietns and I want use to stop working server by usingh its active:=false (button2.click method)?

The most common reason for that to happen is that you are likely starting a synchronized operation to the main thread (or are already in the middle of a sync operation) while deactivating the server from within the main thread at the same time. That is a guaranteed deadlock scenario. Remember that each client runs in its own thread within the server. When the main thread deactivates the server, it is blocked while it waits for the server to finish deactivating, so it cannot process sync requests. The server deactivation waits for all of the client threads to fully terminate. A syncing client thread is blocked while waiting for the main thread to process s sync request, so it cannot terminate. Deadlock occurs. The number of clients does not matter, it could happen even if only 1 client is connected.

To address that, you have a couple of choices:

1. create a worker thread to deactivate the server, instead of having the main thread deactivate it. This frees up the main thread to process sync requests normally, allowing the client threads to terminate normally and the server to fully deactivate normally. For example:

   type
     TShutdownThread = class(TThread)
     protected
       procedure Execute; override;
     end;
   
   procedure TShutdownThread.Execute;
   begin
     MainForm.IdTCPServer.Active := False;
   end;
   
   procedure TMainForm.Button2Click(Sender: TObject);
   begin
     if MainForm.IdTCPServer.Active then
     begin
       with TShutdownThread.Create(False) do
       try
         WaitFor; // internally processes sync requests...
       finally
         Free;
       end;
     end;
   end;
   

2. eliminate thread-blocking syncs wherever possible. Do as much work as you can within the client threads directly, not in the main thread. Especially for operations that your client code does not actually have to wait for a response from the main thread for. If something does not actually need to be synced across thread boundaries, then do not sync it. When you do have to sync with the main thread, use TIdNotify instead of TIdSync whenever possible. TIdNotify is asynchronous, so it does not block the calling thread like TIdSync does, thus avoiding the deactivation deadlock. You just have to be a little more careful with TIdNotify, because it is asynchronous. It is placed into a background queue and executed at a later time, so you have to make sure any objects and data you access with it are still valid when it eventually runs. For that reason, it is best to make TIdNotify implementations that are as self-contained as possible so they do not rely on external things. For example:

   type
     TMemoNotify = class(TIdNotify)
     protected
       FStr: String;
       procedure DoNotify; override;
     public
       class procedure AddToMemo(const Str: string);
     end;
   
   procedure TMemoNotify.DoNotify;
   begin
     MainForm.Memo1.Lines.Add(FStr);
   end;
   
   class procedure TMemoNotify.AddToMemo(const Str: string);
   begin
     with Create do
     begin
       FStr := Str;
       Notify;
       // DO NOT free it!  It is self-freeing after it is run later on...
     end;
   end;
   
   procedure TCliContext.ProcessMsg;
   var
     Msg: string;
   begin
     Msg := Connection.IOHandler.ReadLn;
     TMemoNotify.AddToMemo(Msg);
     ...
   end;
   
   procedure TMainForm.IdTCPServerConnect(AContext: TIdContext);
   begin
     ...
     TCliContext(AContext).Who := ...;
     TMemoNotify.AddToMemo(TCliContext(AContext).Who + ' connected');
     ...
   end;
   
   procedure TMainForm.IdTCPServerDisconnect(AContext: TIdContext);
   begin
     ...
     TMemoNotify.AddToMemo(TCliContext(AContext).Who + ' disconnected');
     ...
   end;
   

can I use TCliContext.BroadcastMessage inside the TCliContext.ProccessMsg just invoking it wihtout any synchronization?

Yes, because the TIdTCPServer.Context and TIdThreadSafeStringList locks are providing adaquate synchronization (they both use TCriticalSection internally). The same applies to TCliContext.SendMessageTo() as well.

can I read (Connection.IOHandler.ReadLn()) in OnConnect method (I want read line with login data and check it in DB then when it is incorrect do disconnection immediatly?

Yes. OnConnect (and OnDisconnect) runs in the same client thread context that OnExecute runs in. TIdTCPServer checks if the socket is still connected after OnConnect exits, before then starting the OnExecute loop, in case OnConnect does decide to disconnect the client.

I read somewhere, using IdSync is sometimes hazard (if anything goes wrong inside using it)

For the most part, TIdSync and TIdNotify are safe to use as long as you use them correctly.

TIdSync, being synchronous, does have deadlock potential if the main thread becomes blocked, that's all.

If you use TIdNotify, make sure you are using an up-to-date version of Indy 10. Some earlier releases of Indy 10 had a memory leak in TIdNotify, but that was recently fixed.

what is better solution to reach global variables or VCL objects?

Globals that are not strictly tied to any given thread should provide their own sychronization when possible. Whether in their own internal code (like your BroadcastMessage() and SendMessageTo() implementations), or via separate locks, like TCriticalSection objects.

VCL objects must be accessed within the main thread only, so if you do not use TIdSync/TIdNotify, you have to use some other form of thread synchronization of your choosing to delegate your code to run in the context of the main thread. This is where separation of UI logic and business logic really comes into play. If possible, you should separate your business data from the UI, then provide safe inter-thread locks around your data operations, then you can have the UI update the data safely when needed, and have your worker threads update the data safely when needed and post asynchronous requests to the UI to display the latest data.