Memoryleaks

[FrankS]

Hallo ich wieder,
ich habe jetzt die Umsetzung eines C# Programms (Textadventure-Framework) nach FPC fertig und es läuft soweit. Allerdings habe ich große Probleme mit dem verhindern von memoryleaks beim beenden des Programms. Ich verstehe nicht so recht wie diese zustande kommen.
Jetzt zu meiner unverschämten Bitte: Vielleicht kann sich mal jemand mein Programm anschauen und mir hilfreiche Hinweise geben.
Zum testen einfach das Programm starten und test eingeben, damit wird eine Komandotestliste abgearbeitet.

Im Anhang mein Projekt inklusive dem C#-Projekt.

Gruß
Frank

[Zvoni]

Bin jetzt nicht durch alles durch, aber mir sind sofort zwei Dinge in GameDataUnit aufgefallen.

1) Ich war der Meinung, dass man einen Destructor "overriden" soll. hab jetzt schon die erste Stelle gefunden, wo das Schlüsselwort "override" in der Klassen-Definition fehlt
2) Du rufst FIrgendwas.Destroy auf --> Soll man nicht machen. Benutze FIrgendwas.Free

EDIT: AUTSCH!!!!
gerade gesehen in "class procedure TGameData.InitFObs();"

Code: Alles auswählen

var
    aStringList: TStringList;
begin
    FObs := TObsList.Create;

    FObs.Add(OB_Acorn,     TThing.Create('acorn', 'ordinary-looking acorn'));
    FObs.Add(OB_Bed,       TThing.Create('bed', 'nasty, rather damp bed', false, true));
    FObs.Add(OB_Bin,       TContainerThing.Create('bin', 'smelly old bin', Mass_MEDIUM, true, true, true, true));
        aStringList := TStringList.Create(); aStringList.CommaText := 'smelly,old';
        FObs.KeyData[OB_Bin].Adjectives := aStringList;
    FObs.Add(OB_Bone,      TThing.Create('bone', 'bone that looks as though it has been gnawed by a dog'));
    FObs.Add(OB_BrassKey,  TThing.Create('key', 'small brass key'));
        aStringList := TStringList.Create(); aStringList.CommaText := 'small,brass';
        FObs.KeyData[OB_BrassKey].Adjectives := aStringList;
//        ......

aStringList wird created, einer Eigenschaft zugewiesen, dann NOCHMAL created (und das kommt noch mehrmals vor)?!?!?!?
Zumal die Eigenschaft "Adjectives" ja anscheinend ein TStringList/TStrings ist. Wieso nicht direkt diese Eigenschaft createn?

Desweiteren Createst du TThing innerhalb der Add-Funktion. Dir ist klar, dass du dann keinerlei Referenz mehr darauf hast???
Zumal anscheinend FObs keinen Parent bekommt.

Alles gesagt: Diesen Code würde ich nichtmal mit der Beisszange anfassen

[Zvoni]

Also wenn du dennoch das irgendwie so lassen willst, würde ich wie folgt vorgehen.
Und das ist nur die erste "Änderung".

UNGETESTET.
Bin mir jetzt nicht sicher, ob "Adjectives" automatisch created wird

Code: Alles auswählen

var
    aStringList: TStringList;
begin
    FObs := TObsList.Create;

    aStringList := TStringList.Create(); //<== Einmal createn

    FObs.Add(OB_Acorn,     TThing.Create('acorn', 'ordinary-looking acorn'));
    FObs.Add(OB_Bed,       TThing.Create('bed', 'nasty, rather damp bed', false, true));
    FObs.Add(OB_Bin,       TContainerThing.Create('bin', 'smelly old bin', Mass_MEDIUM, true, true, true, true));

        aStringList.CommaText := 'smelly,old';  //<==
        FObs.KeyData[OB_Bin].Adjectives.AddStrings(aStringList, True);  //<==

    FObs.Add(OB_Bone,      TThing.Create('bone', 'bone that looks as though it has been gnawed by a dog'));
    FObs.Add(OB_BrassKey,  TThing.Create('key', 'small brass key'));

        aStringList.CommaText := 'small,brass';
        FObs.KeyData[OB_BrassKey].Adjectives.AddStrings(aStringList, True);   //<==
//        ......
    aStringList.Free;  //Zum Schluss freigeben
End;    

Anstatt der "aStringList" vielleicht sogar direkt den overload von AddStrings verwenden, und die Strings direkt übergeben
https://www.freepascal.org/docs-html/rt ... rings.html

Code: Alles auswählen

FObs.KeyData[OB_BrassKey].Adjectives.AddStrings(['small','brass'],True);   //<==

[FrankS]

Hallo Zvoni,
vielen Dank für deine Hinweise.

Ich habe erstmal unbedarft mehr oder weniger die Struktur des C#-Programms übernommen.

Ich dachte und so habe ich es auch getestet (glaube ich wenigstens), daß TFPGMap<TOb,TThing>.Add ein Objekt mit dem Key TOb und einer Kopie der Reference von TThing anlegt.

Mein Problem ist auch, dass ich im erzeugten dump der memoryleaks nicht sehen kann das hier ein Problem vorliegen soll.

Frank

[Zvoni]

Hallo Frank,

wie gesagt: Das alles war nur der erste Blick in einer Unit.
Es war keine Aussage darüber, dass dies die Ursache für die Speicher-Lecks sind.

[FrankS]

Hallo,
ich spare mir mal einen neuen Thread.
Ich habe alle Speicherlecks eliminiert und eine Save- und Loadroutine implementiert.
Läuft soweit auch (dachte ich).
Beim programmieren habe ich einige writeLn Komandos in der Procedure verteilt. Als ich fertig war habe ich diese entfernt und plötzlich gibt es beim Beenden des Programms wieder Speicherlecks.
Es ist völlig egal wieviele Textausgaben und wo diese in der Procedure stehen, sobald eine dieser Textausgaben eine bestimmte Länge über oder unterschreitet gibt es diese Speicherlecks.
Mindestens eine muss in der richtigen Länge vorhanden sein.

Von den Textausgaben mit Kommentar reicht jeweils eine um die Speicherlecks zu erzeugen.

Was macht writeLn?
Ich kann mir einfach das Problem nicht vorstellen.

Code: Alles auswählen

class procedure TGameData.LoadGameData(fs: TFileStream);
var
    objID: TOb;
    roomID: TRm;
    currentClassName: shortstring;
begin
    CleanFObs;
    CleanFMap;
    Player.Free;

    writeLn ('                                                ');
    writeLn ('                                                ');
    writeLn ('                                                ');
    writeLn ('          ');                                       // Erzeugt das Problem
    writeLn ('                                                ');
    writeLn ('                                                ');
    writeLn ('                                                ');
    writeLn ('                                                                                          ');  // Erzeugt das Problem
    writeLn ('                                                ');

    // --- Die Objekte  -----------------------------------------
    for objID in TOb do begin
        fs.ReadBuffer(currentClassName, sizeof(currentClassName));
        case currentClassName of
            'TThing':               FObs.Add(objID,TThing.CreateFromStream(fs));
            'TGenericThing':        FObs.Add(objID,TGenericThing.CreateFromStream(fs));
            'TInteractiveActor':    FObs.Add(objID,TInteractiveActor.CreateFromStream(fs));
            'TContainerThing':      FObs.Add(objID,TContainerThing.CreateFromStream(fs));
            'TLockableThing':       FObs.Add(objID,TLockableThing.CreateFromStream(fs));
        else
            writeLn('ERROR NOT HANDLED TObsList Object ---> ', currentClassName);
        end;
    end;

    writeLn ('                                                ');
    writeLn ('          ');                                       // Erzeugt das Problem
    writeLn ('                                                ');


    // --- Die Räume  -------------------------------------------
    for roomID in TRm do begin
        if roomID <> RM_NOEXIT then begin
            fs.ReadBuffer(currentClassName, sizeof(currentClassName));
            FMap.Add(roomID,TRoom.CreateFromStream(fs));
		end;
	end;

    // --- Der Player  ------------------------------------------
    fs.ReadBuffer(currentClassName, sizeof(currentClassName));
    FPlayer := TActor.CreateFromStream(fs);

    // --- Die Referenzen  --------------------------------------
    CorrectReferences();

end;

Gruß
Frank

[FrankS]

Hallo,
habe soeben festgestellt, daß ich speichern und laden mit der selben exe ausführen muß.
Dann läuft es.
Also suchen warum dem so ist.

Frank

[Warf]

2) Du rufst FIrgendwas.Destroy auf --> Soll man nicht machen. Benutze FIrgendwas.Free

Nein!

Free basiert auf der Annahme das es bevorzugt ist das ein Programm in einem Undefinierten zustand weiter läuft und potentiell Daten zerstört als das es sauber mit einem zurückverfolgbaren stack trace crasht. Crashes sind zwar unschön, aber wenn das Programm in einem Status ist in dem es nicht sein sollte ist die alternative das es dinge tut die es nicht soll. Deshalb crashen unbehandelte Exceptions Programme auch für gewöhnlich und warum Sprachen wie Rust die zwar keine Exceptions haben trotzdem eine "panic" haben für undefinierten status der einfach das Programm mit stack trace crasht

Du solltest Free eigentlich nur verwenden wenn:
1. Du dir sicher bist das du bei jedem Destruktoraufruf die variable auf nil setzt
2. In einer situation bist wo nicht klar ist ob das objekt schon gefreed wurde oder noch existiert

In allen anderen Fällen versteckt Free einfach nur use-after-free Fehler die sonst potentiell durch testing gefunden würden. Ich hab mir die Nutzung von Free leider auch angewöhnt und kann die Muscle memory nicht einfach ablegen und Nutz es deshalb selbst zu oft. Aber das heißt nicht das man schlechte Eigenschaften weiterempfehlen sollte

[fliegermichl]

...
Ich hab mir die Nutzung von Free leider auch angewöhnt und kann die Muscle memory nicht einfach ablegen und Nutz es deshalb selbst zu oft. Aber das heißt nicht das man schlechte Eigenschaften weiterempfehlen sollte

Und was wäre dann die zu bevorzugende Variante?

[theo]

Und was wäre dann die zu bevorzugende Variante?

Z.B. https://wiki.freepascal.org/FreeAndNil

[PascalDragon]

Free ist die bevorzugte Variante unabhängig davon was Warf sagt und FreeAndNil ist auch kein Allheilmittel.

[Warf]

Free ist die bevorzugte Variante unabhängig davon was Warf sagt und FreeAndNil ist auch kein Allheilmittel.

Das ist absolut unlogisch. Free ist keine Magie, die Frage Free vs Destroy ist also:

Code: Alles auswählen

myObj.Destroy;
// vs
if myObj <> nil then
  myObj.Destroy;

Wenn das zweite strikt vor dem ersten bevorzugbar wäre, dann müsste das um logisch konsistent zu sein für ALLE methoden gelten. Nimm den folgenden Code:

Code: Alles auswählen

sl := TStringList.Create;
sl.LoadFromFile(Filename);
WriteLn(sl.Text);
sl.Free;

Das ist äquivalent zu:

Code: Alles auswählen

sl := TStringList.Create;
sl.LoadFromFile(Filename);
WriteLn(sl.Text);
if sl<>nil then
  sl.Destroy;

Wenn SL in der letzten Zeile nil sein kann müsste das auch bei den anderen Zeilen gelten, ergo müsste man eigentlich schreiben:

Code: Alles auswählen

sl := TStringList.Create;
if sl<>nil then
  sl.LoadFromFile(Filename);
if sl<>nil then
  WriteLn(sl.Text);
if sl<>nil then
  sl.Destroy;

Ich will dir jetzt keine Position unterstellen, aber ich hab deinen Code gesehen, du schreibst keinen solchen code irgendwo sonst.

Ergo, entweder ist im ersten Beispiel .Free unnötig und .Destroy wäre passender, oder wir alle sollten wie im letzten beispiel Programmieren und würden von den ganzen conditionals dämlich werden

Es gibt kein Argument warum in diesem Beispiel Free statt Destroy verwendet werden sollte. Und ich weiß das das Beispiel oben Absurd ist, das ist der Punkt, die Nutzung von Free ist in den meisten Fällen absolut unnötig. Sollte SL irgendwo innerhalb dieses Codes auf nil fallen, ist das ein Zeichen von z.B. Stack overflows oder sonstigen Fehlverhalten und dann sollte es knallen!

[Warf]

Hier ist übrigens ein ganz Konkretes Beispiel wie Free einen Fehler aktiv versteckt:

Code: Alles auswählen

program Test;

uses Classes;

type
  TVec2F = record
    x, y: Double;
  end;
  TVec3F = record
    x, y, z: Double;
  end;

{$RangeChecks On}
var
  v: TVec2F;
  sl: TStringList;
begin
  sl := TStringList.Create;
  FillChar(v,SizeOf(TVec3F), #00); // Typo Vec3F statt Vec2F -> Stack overflow
  sl.Free;
end.

Hier wird durch einen Stack Overflow (der übrigens durch RangeChecks nicht erkannt wird) sl überschrieben. Also ganz klar ein Fehler der hier stattfindet. Dank Free wird der Fehler aber nicht gefunden. Wenn man Free durch Destroy austauscht crasht das Programm und gibt mir einen Stack Trace über den ich das ganze Debuggen kann um den Fehler zu finden.

Beim Programmieren sollte man so vorgehen das man wenn möglich Fehler vermeidet, aber wenn sich Fehler nicht vermeiden lassen das finden dieser so einfach wie möglich macht. Free macht das Gegenteil, es versucht aktiv Fehler zu verstecken!

PS: FreeAndNil ist übrigens noch schlimmer weil FreeAndNil double Free versteckt:

Code: Alles auswählen

sl := TStringList.Create;
FreeAndNil(sl);
FreeAndNil(sl); // Fehler doppeltes Free

Keine Fehlermeldung obwohl hier ganz klar ein Fehler drin ist