Porting old Delphi2007 application using TServerSocket....

[rlebeau]

As you pointed out the old code uses string as the container even though the data is not necessarily textual.
So if I were to change that to use TIdBytes instead, what would the read function in the thread look like?
Now with FRxData declared as string:

Code:

FRxData: string;
...
FRxData := FConn.IOHandler.ReadString(FConn.IOHandler.InputBuffer.Size);

If I instead use TIdBytes is the read function going to look like this?

Code:

FRxData: TIdBytes;
...
FConn.IOHandler.ReadBytes(FRxData, FConn.IOHandler.InputBuffer.Size, false);

Yes, or simpler:

Code:

FConn.IOHandler.ReadBytes(FRxData, -1, false);

Which will read everything that is currently available at that moment.

Though, in an STX/ETX scenario, I would suggest something more like this instead (as there is currently no TIdBytes version of TIdIOHandler.WaitFor() available):

Code:

LPos := 0;
repeat
  LPos := FConn.IOHandler.InputBuffer.IndexOf(STX, LPos);
  if LPos <> -1 then Break;
  LPos := FConn.IOHandler.InputBuffer.Size;
  FConn.IOHandler.ReadFromSource(True, FConn.IOHandler.ReadTimeout, True);
until False;

FConn.IOHandler.InputBuffer.Remove(LPos+1);

LPos := 0;
repeat
  LPos := FConn.IOHandler.InputBuffer.IndexOf(ETX, LPos);
  if LPos <> -1 then Break;
  LPos := FConn.IOHandler.InputBuffer.Size;
  FConn.IOHandler.ReadFromSource(True, FConn.IOHandler.ReadTimeout, True);
until False;

FConn.IOHandler.ReadBytes(FRxData, LPos-1, false);
FConn.IOHandler.InputBuffer.Remove(1);

// use FRxData as needed...

Or, if you need the STX and ETX bytes to be in the FRxData buffer:

Code:

LPos := 0;
repeat
  LPos := FConn.IOHandler.InputBuffer.IndexOf(STX, LPos);
  if LPos <> -1 then Break;
  LPos := FConn.IOHandler.InputBuffer.Size;
  FConn.IOHandler.ReadFromSource(True, FConn.IOHandler.ReadTimeout, True);
until False;

if LPos > 0 then
  FConn.IOHandler.InputBuffer.Remove(LPos);

LPos := 1;
repeat
  LPos := FConn.IOHandler.InputBuffer.IndexOf(ETX, LPos);
  if LPos <> -1 then Break;
  LPos := FConn.IOHandler.InputBuffer.Size;
  FConn.IOHandler.ReadFromSource(True, FConn.IOHandler.ReadTimeout, True);
until False;

FConn.IOHandler.ReadBytes(FRxData, LPos+1, false);

// use FRxData as needed...

Do I have to set the size of FRxData before calling the read or is that handled inside the read?

ReadBytes() will resize the TIdBytes for you.  If the AAppend parameter is True, the read bytes are appended to the end of the TIdBytes' current allocation.  If the AAppend parameter is False, the TIdBytes is set to just the read bytes by themselves.

Is the synchronized call to the event handler getting a copy of FRxData or FRxData itself?

FRxData itself, since dynamic arrays are reference counted, same as with strings.

Does it have to set back the length of the container to zero after processing the data?

If you set the AAppend parameter of ReadByes() to True, then yes.

This is the section of code I ave problem with there.

Indy clients are not event driven, so there are no such events available, so just get rid of all those handlers altogether.

TIdTCPClient.Connect() is synchronous. It will raise an exception in the calling thread if it fails.

If a read/write IO operation fails, including due to a graceful disconnect by the peer, it will raise an exception in the calling thread.

The compiler stops at the first line complaining about the missing identifier:

Code:

class_SSRemoteClient.pas(110,82) Error: Identifier not found "TErrorEvent"

As it shuld be, since that enum is specific to the old Socket components.  There is no equivalent in Indy.

I have looked at the documentation and found that there is an event OnStatus that gets fired for any status change, so maybe that could replace all 4 of the above separate events?

Not all 4, no.  You could use it for OnConnect and OnDisconnect, but there are status events for pending data waiting to be read, or for errors.

There seems also to be other events available when looking in Lazarus code completion, here are all I found:
...
Which one should I use for example in order to create and terminate the read thread?

OnConnected (or OnStatus(hsConnected)), and OnStatus(hsDisconnecting), respectively.

Though honestly, I wouldn't bother with the events at all.  As I mentioned earlier, I would simply create the thread after Connect() exits without error (which you are already doing), and then signal the thread to terminate before calling Disconnect() (which you are already doing), and wait for the thread to finish terminating after Disconnect() exits (which you are not doing), eg:

Code:

procedure TTcpClientComm.Disconnect;
begin
  if FReadThread <> nil then
    FReadThread.Terminate;
  try
    FConn.Disconnect;
  finally
    if FReadThread <> nil then
    begin
      FReadThread.WaitFor;
      FreeAndNil(FReadThread);
    end;
  end;
end;

Notice that I know when the socket is commanded to connect but not necessarily when it is disconnected since that can be commanded from the client side.

Then you do know when it would be commanded to disconnect, since that is when it would be calling Disconnect() for itself.

My current implementation of the event driven data reception class using TIdTcpClient shown below.
Does it look sensible now, especially how the thread is created and destroyed in connect/disconnect?

I have a few notes:

- I would suggest making the Data parameter of TCommRxEvent be const to disable unnecessary reference counting.  Same with the WriteString() and WriteData() methods.

- there is no reason for TReadingThread to have its own FOnRxData member.  Give the thread a pointer to the owning TTcpClientComm instead, and then the thread can use the TTcpClientComm.FOnRxData member directly.

- it is not a good idea to use the TIdTCPConnection.Connected() method at all.  It performs a read operation to determine if the socket is still alive, and so that will risk corrupting your communications if the socket is read from by two separate threads at the same time.  If you must have a GetConnected() method in TTcpClientComm then I suggest giving it its own FConnected boolean member that you set to True after calling TIdTCPClient.Connect(), and then set to False when an IO error is detected and/or when you are disconnecting the TIdTCPClient.  And there is no good reason to call Connected() right before performing a read/write operation.  If the socket has been disconnected/lost, the operation will raise an exception.

- your WriteString() method will not work for Unicode strings (Delphi 2009+, or FPC in UnicodeStrings mode).  You should let Indy convert the Data string to a TIdBytes, eg:

Code:

DataBytes := ToBytes(Data, IndyTextEncoding_UTF8);

Alternatively:

Code:

DataBytes := IndyTextEncoding_UTF8.GetBytes(Data);

- your thread's Execute() method can be simplified to this:

Code:

procedure TReadingThread.Execute;
begin
  while not Terminated do
  begin
    // read bytes from FConn up to InputBuffer.Size bytes ...
    FConn.IOHandler.ReadBytes(FRxData, -1, False);
    // process bytes as needed ...
    if Assigned(FOnRxData) then
      Synchronize(DoOnRxData); 
    end;
  end;
end;