Delphi disconnecting TIdTCPClient in worker thread

Question

I need to send a TCP array of bytes via FMX application to a device. I have this interface:

type
  IPacketSend = interface
    procedure SendAsync(const Msg: String; OnSuccess: TSendSuccess; OnError: TSendError);
  end;

I have to use threading to not block the UI. This class actually sends the message, in a very simplified version:

type
  TPacketSenderLAN = class(TInterfacedObject, IPacketSend)
    private      
      FSelf: IPacketSend;
    public      
      procedure SendAsync(const Msg: String; OnSuccess: TSendSuccess; OnError: TSendError);
  end;

implementation

{ TPacketSender<T> }

procedure TPacketSenderLAN.SendAsync(const Msg: String; OnSuccess: TSendSuccess;
  OnError: TSendError);
begin
  TTask.Run(
    procedure
    var
      Client: TIdTCPClient;
      Exc: TObject;
    begin
      Client := TIdTCPClient.Create(nil);
      try
        try
          Client.Host := '192.168.0.213';
          Client.Port := 5200;
          Client.ConnectTimeout := 3500;

          Client.Connect;

          Data := TIdBytes(...);
          Client.Socket.Write(Data);

          TThread.Synchronize(nil,
            procedure
            begin
              OnSuccess;
              FSelf := nil;
            end
          );
        except
          on E: Exception do
            begin
              Exc := AcquireExceptionObject;

              TThread.Synchronize(nil,
                procedure
                begin
                  OnError(Exception(exc).Message);
                  FSelf := nil;
                end
              );
            end;
        end;
      finally
        Client.Free;
      end;
    end
  );
end;

end.

The FSelf variabile is absolutely needed because with FSelf := Self; in the constructor I prevent the reference count to go to 0 when the worker thread executes. In fact I call...

TThread.Synchronize(nil,
  procedure
    begin
      OnSuccess;
      FSelf := nil;
     end
);

... where FSelf := nil; is at the end so that the object is disposed when the job is done. I call it in this way from the code:

var
  PacketSender: IPacketSend;
begin
  PacketSender := TPacketSenderLAN.Create(...);
end;

---

Given the above scenario, my question is:

am I using the TIdTCPClient safely? Do I have to disconnect it?

I don't know if I should call Client.Disconnect; inside the finally block. I think it's not needed because Free will destroy the TIdTCPClient and thus the client will disconnect. Is my code safe?

Answer 1

am I using the TIdTCPClient safely?

Yes, you are.

Data, on the other hand, not so much, since it is not shown as being a local variable, or even a member of the TPacketSenderLAN class, which means it must be a global variable instead, and thus would be subject to multithread concurrency issues. In this case, it should be a local variable.

Do I have to disconnect it?

I would recommend it, yes, particularly before calling your OnSuccess/OnError handlers. If you don't call Disconnect() manually, the TCP connection will not be disconnected until the TIdTCPClient destructor is called. In this code, there is no reason for the TCP connection to remain active while your event handlers are running.

I don't know if I should call Client.Disconnect; inside the finally block.

I would actually suggest adding another try..finally block just to call Disconnect(), eg:

procedure
var
  Client: TIdTCPClient;
  Data: TIdBytes;
begin
  try
    Client := TIdTCPClient.Create(nil);
    try
      Client.Host := '192.168.0.213';
      Client.Port := 5200;
      Client.ConnectTimeout := 3500;

      Client.Connect;
      try      
        Data := TIdBytes(...);
        Client.IOHandler.Write(Data);
      finally
        Client.Disconnect;
      end;
    finally
      Client.Free;
    end;
  except
    on E: Exception do
    begin
      TThread.Synchronize(nil,
        procedure
        begin
          OnError(E.Message);
        end
      );
      Exit;
    end;
  end;

  TThread.Synchronize(nil,
    procedure
    begin
      OnSuccess;
    end
  );
end