Listener for TIdTcpClient

[chkaufmann 17]

My application has to connect to another application. It waits for messages in XML format, delimited with SOH and EOT. I wrote a listener thread. So far it works, but I wonder, if this is the way to do it. Here is my code:

- Is it correct to call ReadLn and then the thread is blocked until any data is sent by the server?

- I cannot test the error handling with SendData because I don't succeed to deactivate the server in the moment when the data is sent. Will it work that way? Or should I check another property of my TIdTcpClient to see if the server was deactivated?
- Do I need more cleanup if I destroy my TTsClient object?

 

Christian
 

type
  TTsClientThread = class(TThread)
  strict private
    FClient   : TIdTCPClientCustom;
    FMessages : IBSQueue<IBSXmlElement>;
    FPartMsg  : String;
  strict protected
    procedure Execute; override;
  public
    constructor Create(AClient: TIdTCPClientCustom; const AMessages: IBSQueue<IBSXmlElement>);
  end;

  TTsClient = class(TIdTCPClientCustom)
  strict private
    FListener : TTsClientThread;
    FMessages : IBSQueue<IBSXmlElement>;
  public
    constructor Create(const AIpAddress: String; APort: Integer);
    destructor Destroy; override;
    function IsAlive: Boolean;
    function Pop(var AMessage: IBSXmlElement): Boolean;
    procedure SendData(const AMessage: IBSXmlElement);
  end;

{ TTsClientThread }

constructor TTsClientThread.Create(AClient: TIdTCPClientCustom; const AMessages: IBSQueue<IBSXmlElement>);
begin
  FClient   := AClient;
  FMessages := AMessages;
  inherited Create;
end;

procedure TTsClientThread.Execute;
var
  s : String;
begin
  while not Terminated do begin
    try
      s := FClient.Socket.ReadLn(Char.EOT, IndyTextEncoding_UTF8).Substring(1);
      if not s.IsEmpty
        then FMessages.Append(NewBSXmlFromString(s));
    except
      on E: EIdSocketError do begin
        Terminate;
        Exit;
      end;
    end;
  end;
end;

{ TTsClient }

constructor TTsClient.Create(const AIpAddress: String; APort: Integer);
begin
  inherited Create(nil);
  FMessages   := TBSGenerics.GenQueue<IBSXmlElement>;
  Host        := AIpAddress;
  Port        := APort;
  Connect;
  FListener := TTsClientThread.Create(Self, FMessages);
end;

destructor TTsClient.Destroy;
begin
  FListener.Free;
  inherited;
end;

function TTsClient.IsAlive: Boolean;
begin
  Result := not FListener.Terminated;
end;

function TTsClient.Pop(var AMessage: IBSXmlElement): Boolean;
begin
  Result := FMessages.Pop(AMessage);
end;

procedure TTsClient.SendData(const AMessage: IBSXmlElement);
begin
  try
    Socket.Write(IndyTextEncoding_UTF8.GetBytes(Char.SOH + AMessage.AsXmlDataString([]) + Char.EOT));
  except
    on E: EIdSocketError
      do FListener.Terminate;
  end;
end;

 

[Remy Lebeau 1421]

11 hours ago, chkaufmann said:

So far it works, but I wonder, if this is the way to do it.

Pretty close.  Though, I do question the decision to derive TTsClient from TIdTCPClientCustom, rather than creating a TIdTCPClient data member internally.  But either way will work.

11 hours ago, chkaufmann said:

- Is it correct to call ReadLn and then the thread is blocked until any data is sent by the server?

Yes.

11 hours ago, chkaufmann said:

- I cannot test the error handling with SendData because I don't succeed to deactivate the server in the moment when the data is sent.

You could simply raise your own exception to see how the code will react.  Either raise it in SendData() itself, or maybe in the client's OnWork... events.

11 hours ago, chkaufmann said:

Will it work that way?

An exception handler around Socket.Write() and Socket.ReadLn() will work, provided an exception is eventually raised.  If the connection is closed gracefully, a subsequent Write()/ReadLn() call will raise an exception immediately.  But, if the connection is lost abnormally, it may (likely will) take awhile for the OS to notice so it can invalidate the connection.  Until that time, no errors will be reported by the socket, and this no exceptions raised.  ReadLn() will happily block the caller waiting for more data that will not arrive, and Write() will happily keep sending data to the socket's internal buffer until it eventually fills up, then Write() will start blocking the caller until the connection is closed/invalidated.

 

In any case, I would suggest not handling EIdSocketError alone, but SysUtils.Exception generally, or at least EIdException.  There are other types of Indy exceptions that could possibly be raised which are not derived from EIdSocketError, for example EIdConnClosedGracefully.  Otherwise, simply don't catch any exception at all.  If SendData() fails, let the caller handle the exception (you should, however, close the client connection if Socket.Write() fails, in case Socket.ReadLn() is blocked).  If ReadLn() fails, TThread will terminate itself if a raised exception escapes from Execute() (the TThread.FatalException property will get set in that case, but the TThread.Terminated property will not).

11 hours ago, chkaufmann said:

Or should I check another property of my TIdTcpClient to see if the server was deactivated?

There is no such property in Indy.  But you might be tempted to use the client's Connected() method, but don't.  Internally it performs a read operation, which may corrupt the listener thread's reading if Connected() is called from outside of the thread.

 

In your IsAlive() method, consider using TThread.Finished instead of TThread.Terminated (unless you override TThread.DoTerminate() to set TThread.Terminated=True if TThread.Execute() exits from an uncaught exception).

11 hours ago, chkaufmann said:

- Do I need more cleanup if I destroy my TTsClient object?

I see that you are not terminating the listening thread if the client is disconnected by your application.  Since your TTsClient class is derived from TIdTCPClientCustom, I would suggest creating the listening thread in an overridden DoOnConnected() method, and terminating and freeing the thread in an overridden DoOnDisconnected() method, eg:

 

type
  TTsClientThread = class(TThread)
  strict private
    FClient   : TIdTCPClientCustom;
    FMessages : IBSQueue<IBSXmlElement>;
    FPartMsg  : String;
  strict protected
    procedure Execute; override;
  public
    constructor Create(AClient: TIdTCPClientCustom; const AMessages: IBSQueue<IBSXmlElement>);
  end;

  TTsClient = class(TIdTCPClientCustom)
  strict private
    FListener : TTsClientThread;
    FMessages : IBSQueue<IBSXmlElement>;
  strict protected
    procedure DoOnConnected; override;
    procedure DoOnDisconnected; override;
  public
    constructor Create(const AIpAddress: String; APort: Integer); reintroduce;
    destructor Destroy; override;
    function IsAlive: Boolean;
    function Pop(var AMessage: IBSXmlElement): Boolean;
    procedure SendData(const AMessage: IBSXmlElement);
  end;

{ TTsClientThread }

constructor TTsClientThread.Create(AClient: TIdTCPClientCustom; const AMessages: IBSQueue<IBSXmlElement>);
begin
  FClient   := AClient;
  FMessages := AMessages;
  inherited Create(False);
end;

procedure TTsClientThread.Execute;
var
  s : String;
begin
  while not Terminated do
  begin
    s := FClient.Socket.ReadLn(Char.EOT, IndyTextEncoding_UTF8).Substring(1);
    if not s.IsEmpty then FMessages.Append(NewBSXmlFromString(s));
  end;
end;

{ TTsClient }

constructor TTsClient.Create(const AIpAddress: String; APort: Integer);
begin
  inherited Create(nil);
  FMessages   := TBSGenerics.GenQueue<IBSXmlElement>;
  Host        := AIpAddress;
  Port        := APort;
  Connect;
end;

destructor TIdTCPConnection.Destroy;
begin
  Disconnect;
  inherited;
end;

procedure TIdTCPConnection.DoOnConnected;
begin
  FListener := TTsClientThread.Create(Self, FMessages);
  inherited;
end;

procedure TIdTCPConnection.DoOnDisconnected;
begin
  if FListener <> nil then
  begin
    FListener.Terminate;
    FListener.WaitFor;
    FreeAndNil(FListener);
  end;
  inherited;
end;

function TTsClient.IsAlive: Boolean;
begin
  Result := (FListener <> nil) and (not FListener.Finished{FListener.Terminated});
end;

function TTsClient.Pop(var AMessage: IBSXmlElement): Boolean;
begin
  Result := FMessages.Pop(AMessage);
end;

procedure TTsClient.SendData(const AMessage: IBSXmlElement);
begin
  try
    Socket.Write(Char.SOH + AMessage.AsXmlDataString([]) + Char.EOT, IndyTextEncoding_UTF8);
  except
    Disconnect;
    // optional: re-raise...
  end;
end;

 

[chkaufmann 17]

Thanks for the reply. I have one final question. First I tried with this call:

 

var
  tmp : TIdBytes;

  FClient.IOHandler.ReadBytes(tmp, -1, False);
  FPartMsg := FPartMsg + IndyTextEncoding_UTF8.GetString(tmp);
  ....

Here I noticed, that if ReadBytes had nothing to read, tmp was not reset to length zero, but once some bytes could be read, tmp was set to the number of bytes. What is the reason for that?

 

Christian
 

[Remy Lebeau 1421]

9 hours ago, chkaufmann said:

var
  tmp : TIdBytes;

  FClient.IOHandler.ReadBytes(tmp, -1, False);
  FPartMsg := FPartMsg + IndyTextEncoding_UTF8.GetString(tmp);
  ....

Here I noticed, that if ReadBytes had nothing to read, tmp was not reset to length zero, but once some bytes could be read, tmp was set to the number of bytes. What is the reason for that?

Setting AByteCount=-1 will read however many bytes are currently available on the connection at that moment, waiting up to the ReadTimeout if the InputBuffer is empty.

 

Setting AAppend=False will fill the TIdBytes with the requested number of bytes (for AByteCount=-1, that is the InputBuffer.Size), expanding the TIdBytes only if its length is less than the requested byte count.

 

So, it makes sense that the length of the TIdBytes would not change if its current length (1+) is >= the requested byte count (0) so memory doesn't need to be reallocated.

 

When using AAppend=False, you should either:

• pre-size the TIdBytes and then request a specific number of bytes (AByteCount > 0)
• set the TIdBytes to nil and let the read size it for you (AByteCount = -1)

 

In your situation, you are mixing both together.  To remedy that, just reset the TIdBytes to nil when you are done using it, before the next read.

 

However, that being said, reading an arbitrary number of bytes will not work well with UTF-8 if there are non-ASCII characters present in the data, since the resulting bytes could straddle the middle of a multi-byte character, causing decoding to throw an error or corrupt the text.  Is there a reason why you are using AByteCount=-1 in this manner?  Do you not know the actual length of the UTF-8 text ahead of reading it?  Why not?  What protocol are you implementing that doesn't let you discover that length before reading+decoding the bytes?  If the text is variable-length, the protocol needs to either specify the text length before the text bytes, or else terminate the text with a unique delimiter.  Either way, you probably shouldn't be using ReadBytes() in this situation to begin with, there are other ways to handle these conditions, depending on the design of the protocol.  Can you provide more information about the layout of the messages you are trying to read?

Edited December 10, 2020 by Remy Lebeau