Server mit Liste aus Records crasht

[Unity]

Hey!

Ich hab ein kleines Problemchen mit einem Serverprogramm.

Clients melden sich per Handshake bei meinem Server an. Ich speichere diese dann in einer Liste mittels Record.
In einem zweiten Thread sende ich alle 20 Sekunden eine Anfrage an alle Clients die in dieser Liste stehen.

Ist der Client noch "aktiv" sendet er eine Antwort welche dann im Haupthread empfangen wird und den Timestamp des letzten Kontakts aktualisiert (mittels for Schleife durch die Liste).

Ist der letzte Kontaxt länger als X Sekunden her, wird der Client aus der Liste gelöscht. Er wird ebenfalls gelöscht, sollte er eine bestimmte Nachricht senden.

Das Problem ist nun, dass der Server ohne irgendwelche Anzeichen einfach crasht.
Ich habe bereits versucht den Fehler durch debug Meldeungen und neuschreiben zu finden. Leider ohne Erfolg.

Ich gehe davon aus, dass der Fehler an der Liste bzw. den Records liegt.
Der Fehler tritt sporadisch auf, ein Muster ist nicht zu erkennen.

Hier nun der Code zu den beschrieben Abläufen:

Delphi-Quellcode:

function TMain.GetUnixTime: int64;
var
  st:_SYSTEMTIME;
  ft:_FILETIME;
begin
  // first get  windows SYSTEMTIME in UTC / GMT
  GetSystemTime(st);
  // now convert to windows FILETIME
  SystemTimeToFileTime(st, ft);
  // now to UNIXTIME
  result := round((int64(ft) - int64(116444736000000000)) / 10000000);
end;

{ ... }


type
  TStrArray = array of string;
  PRecord = ^TRecord;
  TRecord = record
    name: string;
    Desc: string;
    Version: string;
    ID: Integer;
    ComIP: String;
    ComPort: Integer;
    Timestamp: int64;
  end;

{ ... }

private

    List: TList;
    Rec: PRecord;

{ ... }

procedure TMain.CreateList;
begin
  List:=TList.Create;
end;

{ ... }


{ Speichern der Clients: }

New(Rec);

Rec.Version := sVersion;
Rec.name := sName;
Rec.ServerComIP := ABinding.PeerIP;
Rec.ServerComPort := ABinding.PeerPort;
Rec.Timestamp := GetUnixTime();
List.Add(Rec);

{ Thread zum senden der Anfrage: }

function Thread(p: Pointer): Integer;
var i: Integer;
begin
  while (1=1) do begin
    if Main.List.Count >= 1 then begin
      for i:=0 to (Main.List.Count-1) do begin
            if ((Main.GetUnixTime() - TRecord(Main.List[i]^).Timestamp) > 50) then begin
                Dispose(PRecord(Main.List[i]));
                Main.List.Delete(i);
            end
            else begin
              if (Main.isCreated) then begin
                Main.UdpServer.Send(TRecord(Main.List[i]^).ComIP, TRecord(Main.List[i]^).ComPort, #$2f#$00#$03#$08);
              end
            end
      end;
    end;
    Sleep(20000);
  end;
    result:=0;
end;

{ Aktualisierung des Timestamps: }

for i:=0 to List.Count-1 do begin
        if ((TRecord(List[i]^).ComIP = ABinding.PeerIP) and (TRecord(List[i]^).ComPort = ABinding.PeerPort)) then begin
            TRecord(List[i]^).Timestamp := GetUnixTime();
            break;
        end;
end;

{ Löschen bei Abmeldung: }

for i:=0 to List.Count-1 do begin
        if ((TRecord(List[i]^).ComIP = ABinding.PeerIP) and (TRecord(List[i]^).ComPort = ABinding.PeerPort)) then begin
          Dispose(PRecord(Main.List[i]));
          List.Delete(i);
          break;
        end;
end;

Meine Vermutung war, dass beim Löschen aus der Liste etwas schief geht. Ich habe es darauf hin mit und ohne Dispose versucht... hat aber nichts gebracht.

Über Ideen würde ich mich freuen

Grüße,
Unity

[Neutral General]

Hi,

Du musst "rückwärts löschen". Lass die Schleifen rückwärts laufen, also von Count-1 downto 0. Dann sollte es funktionieren. Beim genaueren Überlegen, versteht man auch warum das so ist ;)

[Unity]

Hi!

Danke für deine Antwort.

Ich habe es mal eingebaut und werde es mal testen.



Ich kann es aber denoch nicht nachvollziehen.
Nachdem der Eintrag gelöscht wurde, wird die Schleife durch das break; unterbrochen.
Sollte es da nicht egal sein ob ich vorwärts oder rückwärts Zähle?


Edit:
Und wie sieht's mit dem Dispose aus? Kann ich das drin lassen?


Grüße,
Unity

[Neutral General]

mh das break; habe ich geschickt überlesen....

Sehe ich das richtig, dass "Main" nicht innerhalb des Threads deklariert wurde, sondern außerhalb im MainThread?

Dann würde ich 2 Sachen vorschlagen (Bin nicht sooo der Thread-Profi, aber ein Versuch ist es ja mal Wert):

Übergib Main dem Threads über den Pointer-Parameter und greife dann darauf zu. Also ungefähr so:

Delphi-Quellcode:

// Kenn die Parameter nicht auswendig. Ich begrenze es mal auf
// 1. Pointer zur Threadfunktion
// 2. Parameter (p)

BeginThread(@Thread,Main);

function Thread(p: Pointer): Integer;
var i: Integer;
    ThreadMain: TMain; //Ka was fürn Typ Main hat.
begin 
  if TObject(p) is TMain then
    ThreadMain := TMain(p)
  else
    exit;

  while (1=1) do begin
    if ThreadMain.List.Count >= 1 then begin  
      // ...
    end;
      // ...
  end;
end;

Wenn du nicht auf die VCL verzichtest, könntest du es auch mal mit einer Ableitung von TThread probieren und deine ganzen Aufrufe mit Synchronize synchronisieren.


Edit Nr. 12+13 (?!): Habe gemerkt, dass folgender Abschnitt eine ganze Menge Unsinn enthält. Da ich allerdings müde bin (2:52 Uhr) und ich nicht nochmehr Mist erzählen will, werde ich das evtl morgen berichtigen. :roll: :mrgreen:


Was anderes: Du könntest eine Klasse von TList ableiten, die sich selbst um die Freigabe der Daten hinter einem Pointer kümmert:

Delphi-Quellcode:

interface

type
  TListEx = class(TList)
  protected
    procedure Notify(Ptr: Pointer; Action: TListNotification); override;
  end;

implementation

{ TListEx }

procedure TListEx.Notify(Ptr: Pointer; Action: TListNotification);
begin
  if Action = lnDeleted then
    Dispose(Ptr);
end;

Dann würde ein:

Delphi-Quellcode:

Main.List.Delete(i);

reichen ;) (Ist allerdings (noch) ungetestet)

Edit:
Also funktionieren tut es, aber ich habe gemerkt, dass man, wenn man keine Memory-Leaks verursachen will, die Strings im Record manuell freigeben muss. D.h. SetLength(Str,0) oder Str := ''; So ist es zumindest bei meinem Test-Record gewesen.
Dieses Problem verschwindet allerdings, wenn man PChars statt Strings benutzt. Die PChars muss man auch nicht manuell freigeben. (Außer, man reserviert für sie manuell Speicher, was man aber meistens nicht tut (WinAPIs ausgenommen))

Und weil ich so gerne editiere, fasse ich mal meine Aussagen zusammen^^:

:arrow: Main über den p-Parameter der Threadfunktion übergeben
:arrow: Ggf. VCL-Threads benutzen und Synchronize benutzen
:arrow: Meine TListEx benutzen :stupid:
:arrow: In dem Record PChars statt Strings benutzen.

[sx2008]

Ich sehe da jede Menge Dinge in deinem Sourcecode, nicht objektorientiert sind.
1.) mach aus dem Record eine Klasse und gibt der Klasse einen vernüftigen Namen (TConnectionData)
2.) statt TList verwende TObjectList. Damit werden die Objekte automtisch freigegeben, wenn du sie aus der Liste löschst
Mit deiner Procedure TListEx.Notify kannst du dir sehr leicht in den Fuß schiessen; überlass' das den getesteten KLassen aus der VCL (damit meine ich TObjectList) und du bist das Problem los
3.) du startest Threads direkt über die Windows-API anstatt die TThread-Klasse zu benützen.
das ist aber kurzsichtig.
Wenn du eine eigene Threadklasse von TThread ableitest, dann kannst du nach dem erzeugen die Objektliste mitgegeben

Delphi-Quellcode:

TWatchdogThread = class(TThread)
...
  property ConnectionList:TObjectList read FConnectionList write FConnectionList;
end;

Damit brauchst du nicht mehr auf die globale Variable "Main" zuzugreifen.
Erkennst du den Vorteil?
Vorher: der Code ist fest mit dem Formular TMain und der Variable Main verbandelt.
Nachher: die Threadklasse bekommt von aussen gesagt was sie tun soll; ein Bezug zum Formular besteht nicht mehr.

Wenn du das alles beachtest wird aus einem hässlichen Code ein wesentlich schönerer und sicherer Code.

Delphi-Quellcode:

procedure TWatchdogThread.Execute;
var
  count : integer;
begin
  count := 0;
  while not Terminated do
  begin
    Inc(count);
    if (count mod 20) = 0 then
    begin
      CheckOldConnections;  // prüfe alte Verbindungen uns lösche ggf.
      RenewTimestampConnections;
    end;
    // dein sleep(20000) ist ganz schlecht
    // das bedeutet, dass dein Thread bis zu 20s keine Reaktion auf Terminierung von Aussen zeigt
    // Prozesse, deren Threads sich nicht in angemessener Zeit beenden lassen können zu Zombieprozessen werden!
    // deshalb ist es besser 20mal eine Sekunde zu warten, dann reagiert der Thread einigermassen flüssig
    Sleep(1000);
  end;
end;

Ich habe einen Teil der Arbeit in die Methoden CheckOldConnections und RenewTimestampConnections verlegt.
Dadurch wird die Struktur klarer und es ist nicht alles in einer grossen unübersichtlichen Funktion.