Openions about this udp server - Madammar - 12-02-2019
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 ?
RE: Openions about this udp server - rlebeau - 12-02-2019
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.
|