TIdServer, about synchronization again

indy, synchronization

Solution

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:

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;

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.

Problem

Original source