Openions about this udp server

[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.