Openions about this udp server

[Madammar]

i have built a udp server for multibale streaming and i need an expert openion about it 

if it reliable or it can be enhanced 

Code:

unit udpaudio;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls, Vcl.ExtCtrls, IdUDPServer,
  IdGlobal, IdSocketHandle, IdBaseComponent, IdComponent, IdUDPBase, DateUtils;

type
  TForm1 = class(TForm)
    Panel1: TPanel;
    Button1: TButton;
    Button2: TButton;
    Edit1: TEdit;
    udpserver: TIdUDPServer;
    Memo1: TMemo;
    procedure Button1Click(Sender: TObject);
    procedure Button2Click(Sender: TObject);
    procedure FormCreate(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure udpserverUDPRead(AThread: TIdUDPListenerThread;
      const AData: TIdBytes; ABinding: TIdSocketHandle);
  private
    procedure AddnewClient(ipada: string; porta, serverprta: integer);
    procedure Leave(ipada: string; porta, serverprta: integer);
    procedure broadcatstoClients(ABinding: TIdSocketHandle; ipada: string; porta,
    serverprta: integer; const AData: TIdBytes);
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation
uses Constant, crypto;

{$R *.dfm}

procedure TForm1.Button1Click(Sender: TObject);
var
Binding: TIdSocketHandle;
SQL : string;
Row_Count : integer;
begin
udpserver.DefaultPort := StrToInt(Edit1.Text);
udpserver.Bindings.Clear;
Binding := udpserver.Bindings.Add;
Binding.IP := '0.0.0.0';
Binding.Port := 5458;


udpserver.Active := false;
udpserver.Active := True;






end;

procedure TForm1.Button2Click(Sender: TObject);
begin
udpserver.Active := false;
AllAUDIO.Clear;
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
if not Assigned(AllAUDIO) then
begin
  AllAUDIO := TThreadList.Create;
end;


end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
if Assigned(AllAUDIO) then
begin
  FreeAndNil(AllAUDIO);
end;
end;




procedure TForm1.broadcatstoClients(ABinding: TIdSocketHandle; ipada : string; porta, serverprta : integer; const AData: TIdBytes);
var
AUDIOLIST : TAUDIOLIST;
c : integer;
List : Tlist;
begin


if not Assigned(AllAUDIO) then
begin
  exit;
end;


list := AllAUDIO.LockList;
try


for c := 0 to list.Count - 1 do
begin
AUDIOLIST := list.Items[c];

if (AUDIOLIST.userserverPort = serverprta)
And (AUDIOLIST.Logged = 'YES')
And (AUDIOLIST.userport <> porta) then
begin

Abinding.SendTo(AUDIOLIST.userip, AUDIOLIST.userport, Adata);

end;




end;





finally
  AllAUDIO.UnlockList;
end;



end;

procedure TForm1.Leave(ipada : string; porta, serverprta : integer);
var
AUDIOLIST : TAUDIOLIST;
i : integer;
List : Tlist;
begin


if not Assigned(AllAUDIO) then
begin
  exit;
end;


list := AllAUDIO.LockList;
try




for I := 0 to list.Count - 1 do
begin
AUDIOLIST := list.Items[I];

if (AUDIOLIST.userport = porta)
And (AUDIOLIST.userip = ipada)
And (AUDIOLIST.userserverPort = serverprta) then
begin

if (AUDIOLIST.Logged = 'YES') then
begin
list.Delete(i);
end;

break;

end;


end;





finally
  AllAUDIO.UnlockList;
end;



end;






procedure TForm1.AddnewClient(ipada : string; porta, serverprta : integer);
var
AUDIOLIST : TAUDIOLIST;
begin

if not Assigned(AllAUDIO) then
begin
  exit;
end;

list := AllAUDIO.LockList;
try






AUDIOLIST := TAUDIOLIST.Create;
AUDIOLIST.userip := ipada;
AUDIOLIST.userport := porta;
AUDIOLIST.userserverPort := serverprta;
AUDIOLIST.Logged := 'YES';


AllAUDIO.Add(AUDIOLIST);



finally
  AllAUDIO.UnlockList;
end;



end;

procedure TForm1.udpserverUDPRead(AThread: TIdUDPListenerThread;
  const AData: TIdBytes; ABinding: TIdSocketHandle);
var
I : integer;
datastring, command, CMDID: string;
ParamsCount: integer;
Params: TArray<String>;
isstream  : string;
begin

datastring := bytestostring(AData);

isstream := 'YES';

if datastring[1] = '1' then
begin


datastring := Copy(datastring, 2, MaxInt);

command := Decryptstrs(datastring);

if (command.Length > 0) And (Pos('~', command) > 0) then
begin
  Params := command.Split(['~']);
end;


ParamsCount := 0;

ParamsCount := Length(Params);

if ParamsCount > 0 then
begin
CMDID := Params[0];
end;


if ParamsCount = 1 then
begin

if CMDID = 'Enter' then
begin
isstream := 'NO';
AddnewClient(ABinding.PeerIP, ABinding.PeerPort, ABinding.Port);

end;

end;


end else

begin
CMDID := copy(datastring,1,pos('~',datastring)-1);//parse

if CMDID = 'Leave' then
begin
isstream := 'NO';
Leave(ABinding.PeerIP, ABinding.PeerPort, ABinding.Port);
end else

begin
if (isstream <> 'NO') then
begin
broadcatstoClients(Abinding, ABinding.PeerIP, ABinding.PeerPort, ABinding.Port, AData);
end;

end;



end;




end;

end.

is it an efficient way to be written like that ?

[rlebeau]

You need to deactivate the server BEFORE you mess around with its Bindings collection.

The DefaultPort is useless in this example since you are defining your own Binding with a Port assigned. The DefaultPort is only used when a new Binding is initially created, which you then overwrite. So either:

- use the DefaultPort by itself and let the server create its own Bindings entries internally

Code:

procedure TForm1.Button1Click(Sender: TObject);
var
  Binding: TIdSocketHandle;
begin
  udpserver.Active := false;

  udpserver.Bindings.Clear;
  udpserver.DefaultPort := StrToInt(Edit1.Text);

  udpserver.Active := True;
end;

- define your own Bindings entries and don't do anything with the DefaultPort at all.

Code:

procedure TForm1.Button1Click(Sender: TObject);
var
  Binding: TIdSocketHandle;
begin
  udpserver.Active := false;

  udpserver.Bindings.Clear;
  Binding := udpserver.Bindings.Add;
  Binding.IP := '0.0.0.0';
  Binding.Port := StrToInt(Edit1.Text);

  udpserver.Active := True;
end;

- use a combination of the two:

Code:

procedure TForm1.Button1Click(Sender: TObject);
begin
  udpserver.Active := false;

  udpserver.Bindings.Clear;
  udpserver.DefaultPort := StrToInt(Edit1.Text);
  udpserver.Bindings.Add.IP := '0.0.0.0';

  udpserver.Active := True;
end;

Aside from that, in your Leave() method, it doesn't make sense to make the call to list.Delete(i) be conditional on the AUDIOLIST.Logged value. The client is leaving, get rid of the list entry for that client, period.

You should also consider using a Timer to periodically get rid of any clients that have been idle for a long time, in case a client dies without ever sending your server a LEAVE command. You should add another command to your protocol, such as KEEPALIVE, that each client should be required to send periodically if it wants to stay logged into your server. This is especially important in UDP, which has no persistent connections like in TCP.

Also, you are not freeing any active clients still in your list when the server is being deactivated or the Form is being closed/destroyed. You might consider using a TIdThreadSafeObjectList instead of a TThreadList so you don't have to worry about that.

Inside your udpserverUDPRead() method, your isstream variable is useless and can be removed completely:

Code:

procedure TForm1.udpserverUDPRead(AThread: TIdUDPListenerThread;
  const AData: TIdBytes; ABinding: TIdSocketHandle);
var
  I : integer;
  datastring, command, CMDID: string;
  ParamsCount: integer;
  Params: TArray<String>;
begin
  datastring := BytesToString(AData);

  if TextStartsWith(datastring, '1') then
  begin
    datastring := Copy(datastring, 2, MaxInt);
    command := Decryptstrs(datastring);

    if (command.Length > 0) And (Pos('~', command) > 0) then
    begin
      Params := command.Split(['~']);
    end;

    ParamsCount := Length(Params);

    if ParamsCount > 0 then
    begin
      CMDID := Params[0];
    end;

    if ParamsCount = 1 then
    begin
      if CMDID = 'Enter' then
      begin
        AddnewClient(ABinding.PeerIP, ABinding.PeerPort, ABinding.Port);
      end;
    end;
  end else
  begin
    CMDID := Fetch(datastring, '~');//parse

    if CMDID = 'Leave' then
    begin
      Leave(ABinding.PeerIP, ABinding.PeerPort, ABinding.Port);
    end else begin
      broadcatstoClients(ABinding, ABinding.PeerIP, ABinding.PeerPort, ABinding.Port, AData);
    end;
  end;
end;

That said, I question your choice to use a text-based protocol when a binary-based protocol would make more sense, unless you are expecting users to type commands to your server manually for testing purposes.