Indy 10 TCP server

Question

After a lot of searching I thought Indy TCP server would be the best to use on Instant messenger server I am working on. The only issue I am facing right now is broadcasting and forwarding message to other connected client, sending back response to the same client seems ok and doesn't hangs up other clients activity, but for forwarding message to other clients the mechanism that I know of is by using the aContext.locklist, and iterating between the connection list to find the client connection that is to receive the data.

The problem here I think is that it freezes the list and doesn't process other clients requests until the unlocklist is called. So will it not hurt the performance of the server? locking the list and iterating between connections for forwarding each message (as this is what happens very often in a messenger). Is there any better way to do this?

I am using Indy 10 and Delphi 7

Code for broadcast:

Var tmpList: TList;
    i: Integer;
Begin
tmpList := IdServer.Contexts.LockList;

For i := 0 to tmpList.Count Do Begin
  TIdContext(tmpList[i]).Connection.Socket.WriteLn('Broadcast message');
End;
IdServer.Contexts.UnlockList;

Code for forwarding message:

Var tmpList: TList;
  i: Integer;
Begin
  tmpList := IdServer.Contexts.LockList;

  For i := 0 to tmpList.Count Do Begin
    If TIdContext(tmpList[i]).Connection.Socket.Tag = idReceiver Then
      TIdContext(tmpList[i]).Connection.Socket.WriteLn('Message');
  End;
  IdServer.Contexts.UnlockList;

Upsilon

You expecting us to psychically debug your code? – Ferry

Sorry, i actually thought of it as a usual problem and not a problem with code. – Upsilon

Answer 1

Yes, you have to loop through the Contexts list in order to broadcast a message to multiple clients. However, you do not (and should not) perform the actual writing from inside the loop. One, as you already noticed, server performance can be affected by keeping the list locked for awhile. Two, it is not thread-safe. If your loop writes data to a connection while another thread is writing to the same connection at the same time, then the two writes will overlap each other and corrupt your communications with that client.

What I typically do is implement a per-client outbound queue instead, using either the TIdContext.Data property or a TIdServerContext descendant to hold the actual queue. When you need to send data to a client from outside of that client's OnExecute event, put the data in that client's queue instead. That client's OnExecute event can then send the contents of the queue to the client when it is safe to do so.

For example:

type
  TMyContext = class(TIdServerContext)
  public
    Tag: Integer;
    Queue: TIdThreadSafeStringList;
    ...
    constructor Create(AConnection: TIdTCPConnection; AYarn: TIdYarn; AList: TThreadList = nil); override;
    destructor Destroy; override;
  end;

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

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

.

procedure TForm1.FormCreate(Sender: TObject);
begin
  IdServer.ContextClass := TMyContext;
end;

procedure TForm1.IdServerConnect(AContext: TIdContext);
begin
  TMyContext(AContext).Queue.Clear;
  TMyContext(AContext).Tag := ...
end;

procedure TForm1.IdServerDisconnect(AContext: TIdContext);
begin
  TMyContext(AContext).Queue.Clear;
end;

procedure TForm1.IdServerExecute(AContext: TIdContext);
var
  Queue: TStringList;
  tmpList: TStringList;
begin
  ...
  tmpList := nil;
  try
    Queue := TMyContext(AContext).Queue.Lock;
    try
      if Queue.Count > 0 then
      begin
        tmpList := TStringList.Create;
        tmpList.Assign(Queue);
        Queue.Clear;
      end;
    finally
      TMyContext(AContext).Queue.Unlock;
    end;
    if tmpList <> nil then
      AContext.Connection.IOHandler.Write(tmpList);
  finally
    tmpList.Free;
  end;
  ...
end;

.

var
  tmpList: TList;
  i: Integer;
begin
  tmpList := IdServer.Contexts.LockList;
  try
    for i := 0 to tmpList.Count-1 do
      TMyContext(tmpList[i]).Queue.Add('Broadcast message');
  finally
    IdServer.Contexts.UnlockList;
  end;
end;

.

var
  tmpList: TList;
  i: Integer;
begin
  tmpList := IdServer.Contexts.LockList;
  try
    for i := 0 to tmpList.Count-1 do
    begin
      if TMyContext(tmpList[i]).Tag = idReceiver then
        TMyContext(tmpList[i]).Queue.Add('Message');
    end;
  finally
    IdServer.Contexts.UnlockList;
  end;
end;

Dole

Thanks for the heads-up on thread safety, i will use it that way, but this arises one more question and the original question is still un-answered.So for writing outside the serverexecute procedure i should implement a queue, and for the operations with in the serverexecute procedure e.g a direct response to sender client, i can just perform the task after the queue check you did in server execute? or should i write to the queue for that too? And the question that it seems however ok to iterate through all the connections for broadcast but... – Upsilon

...but for a simple message forwarding from one client to another or file transfers the iteration each time wouldn't be stupid? can i declare a class for each client with their ip and a TIdContext variable and assign each client's context to it? would it be safe? – Upsilon

My answer does address your original question - the best way to do a broadcast/forward, with performance and thread safety in mind, is to use a per-client queue. Whether broadcasting or forwarding, you still have to loop through the Contexts list to find the client(s) you want to send to (unless you implement your own thread-safe lookups, such as grouping multiple TIdContext pointers together). The queue helps to minimize the amount of time the Contexts list needs to be locked by offloading the bulk of the work to the OnExecute event of each client... – Dole

... OnExecute can do whatever it needs to do with its TIdContext client, including reading responses to queued messages after they have been flushed to the connection. – Dole

One another dumb question, how would i add data of another datatype to the queue? like TIdBytes or something... Thanks – Upsilon

Using TIdThreadSafeStringList was just an example. You can use whatever you want for the queue as long as there is a thread-safe lock around it whenever you access it. For instance, a TThreadList containing pointers to records, where the records contain your actual data. – Dole

Yep i did it that way, thanks for clearing up things :). Much appreciated. – Upsilon

So you have to loop for reaching everyone... That looks creepy. Isn't it much better to use UDP which is more simpler to use and more faster? – Taitaichung

TCP does not support broadcasting, so you have to loop over all of your active connections if you want to send the same data to everyone. No, UDP is not necessarily a better choice. Sure, UDP can simplify your code (a single send can broadcast to multiple receivers), but UDP does not guarantee that data will actually arrive at the destination at all, and even if it did, UDP does not guarantee that data will arrive in the same order it was sent. It is faster only because UDP does not acknowledge data, like TCP does. Faster does not always mean better. – Dole

@RemyLebeau I finished my case, I will use them both! ;-) #18070448 See if you agree with me... at least for my use case. – Taitaichung