Porting old Delphi2007 application using TServerSocket....

[rlebeau]

OK, so then if there is a WaitFor() in the code then it will stay here most of the time?

Yes, if your traffic is light.

Even when the timeout hits it will come back here, right (since it is immediately re-called).

Yes.

Is this call coming from a separate thread so it won't slow down the rest of the application, for example if there are two clients connected...

Yes.  TIdTCPServer is multi-threaded.  Each client runs in its own thread.  The OnConnect, OnDisconnect, OnExecute, and OnException events are fired in the context of those threads.  That is why I said earlier that your Log3RServer logic needed to be thread-safe, since you could have multiple client threads trying to log at the same time.

OK, then I will probably have a first check if there are at least 3 bytes in there (STX-Databyte-ETX) before going on. It would make the OnExecute exit much quicker most of the times

That is not necessary.  That will just lead to busy loops that eat up CPU cycles.  It would be better to just let the event handler block waiting for the message terminator to arrive.  The thread will be in an efficient sleep state during that time.  Stop thinking of TIdTCPServer in terms of events, like with TServerSocket.  Indy is a blocking library, so think in terms of functionality instead.  You want to process a complete message, so read a complete message and process it.  Let Indy block while it is waiting for the complete message to arrive.  Read a message, process it, exit, repeat.  That is how OnExecute is intended to be used in most situations.

I then also think I need to address the case when the server is shut down with clients connected...
But I am not sure how to handle that precisely...

For the most part, TIdTCPServer handles that for you.  Simply deactivate the server (set Active=False) and it will disconnect all active clients for you.

In the case of socket operations that are blocked, they will simply fail when their connections are closed, typically raising exceptions.  Let those exceptions escape your event handlers so the server can terminate the client threads properly.  If you want to catch the exceptions (for logging, cleanup, etc), be sure to re-raise them when you are done, or at least Exit the handlers.

It is important that during a shutdown, execution flow return to TIdTCPServer.  The Active property setter blocks the calling thread waiting for all client threads to terminate.

Which means a gotcha to watch out for is if your event handlers synchronize with the same thread that is deactivating TIdTCPServer.  That will cause a deadlock if you use a synchronous sync, like TThread.Synchronize(), TIdSync, SendMessage(), etc.  In which case, you can either

• use an asynchronous sync instead, like TThread.Queue(), TIdNotify, Post(Thread)Message(), etc.
  
• deactivate the server from another thread, so that the thread which processes the syncs is not blocked.
  

Code:

destructor TRemoteServer.Destroy;
begin
  if FServer.Active then
  begin
    if ClientCount > 0 then
    begin
      //Need to disconnect the clients so they get the message....
      //What to do here?

    end;
    FServer.Active := false;
  end;
  FServer.Free;
  inherited Destroy;
end;

There is nothing to do there.  Just deactivate the server:

Code:

destructor TRemoteServer.Destroy;
begin
  FServer.Active := false; // <-- optional - TIdTCPServer's destructor will do this for you!
  FServer.Free;
  inherited Destroy;
end;

On the other hand, if you want to actually send a message to each client during a shutdown, that is a different matter. You would have to manually loop through the TIdTCPServer.Contexts list sending your messages to each active client as needed.

Which I don't generally suggest doing when your event handlers also send their own messages (replies to requests, etc), as this risks corrupting your communications having 2 separate threads sending messages to a given client potentially at the same time. Unless you synchronize your sends. But, the other problem with doing this kind of loop is that sending messages in a loop in another thread risks crashing the loop if an error occurs, or worse deadlocking the loop if it encounters a misbehaving client. That, and also the sends will be done in serial rather than in parallel (which is not really a problem when you have only a few clients connected, but it may be an issue if you have a lot of clients connected).

The alternative would be to set a flag somewhere during shutdown and then have your OnExecute handler look at that flag periodically when it is safe to do so (ie, between readings of new client messages), and if the flag is set then send a message as needed before disconnecting the client. That way, everything is kept in the context of the client's thread, and each client thread can handle its own shutdown.