Memory-Leaks in TEncoding

[freimatz]

Hallo zusammen,
ich bin auf der Suche nach Memory-Leaks. Dazu verwende ich madexcept, spielt aber für meine Frage wohl keine Rolle. Ich meine in TEncoding etwas gefunden zu haben, bin mir aber nicht sicher. Also ...
Bei debuggen (in SysUtils Tokio) komme ich da durch:

Delphi-Quellcode:

class function TEncoding.GetBufferEncoding(const Buffer: TBytes; var AEncoding: TEncoding;
  ADefaultEncoding: TEncoding): Integer;
...
var
  Preamble: TBytes;
begin
  Result := 0;
  if AEncoding = nil then
  begin
    // Find the appropraite encoding
    if ContainsPreamble(Buffer, TEncoding.UTF8.GetPreamble) then
      AEncoding := TEncoding.UTF8
    else if ContainsPreamble(Buffer, TEncoding.Unicode.GetPreamble) then
...
end;

Dort bei "TEncoding.Unicode.GetPreamble" rein komme ich zu

Delphi-Quellcode:

class function TEncoding.GetUnicode: TEncoding;
var
  LEncoding: TEncoding;
begin
  if FUnicodeEncoding = nil then
  begin
    LEncoding := TUnicodeEncoding.Create;
    if AtomicCmpExchange(Pointer(FUnicodeEncoding), Pointer(LEncoding), nil) <> nil then
      LEncoding.Free;
{$IFDEF AUTOREFCOUNT}
    FUnicodeEncoding.__ObjAddRef;
{$ENDIF AUTOREFCOUNT}
  end;
  Result := FUnicodeEncoding;
end;

Bei "TUnicodeEncoding.Create" wird Speicher erzeugt. Bei LEncoding.Free; komme ich nicht vorbei. Das wird dann bei Result übergeben und weiter oben nirgendwo freigegeben. Ich sehe auch nicht dass es eine Klasse mit Referenzzählung wäre.
Was sehe ich falsch? Oder ist das wirklich ein Leak?

[Der schöne Günther]

Das ist in Ordnung: GetUnicode() erstellt lokal erst einmal in TEncoding und legt es in LEncoding ab. Dann schaut er ob TEncoding.Unicode (class var FUnicodeEncoding) schon belegt ist. Wenn ja dann sagt er "Ok, dann halt nicht" und gibt sein LEncoding direkt wieder frei. Wenn nicht, dann schreibt er in class var FUnicodeEncoding die Referenz auf LEncoding.

So viel zum Thema Delphi sei immer super lesbar

Der Klassendestruktor von TEncoding ruft FreeEncodings() auf, das gibt am Programmende auch das TEncoding.Unicode wieder frei.

[Schokohase]

Das was man dort sieht ist die übliche Vorgehensweise für die thread-safe und lock-free Erstellung einer (hier globalen) Instanz.

Mit TInterlocked könnte man das noch etwas übersichtlicher gestalten:

Delphi-Quellcode:

class function TEncoding.GetUnicode: TEncoding;
var
  LEncoding: TEncoding;
begin
  if FUnicodeEncoding = nil then
  begin
    LEncoding := TUnicodeEncoding.Create;
    try
      LEncoding := TInterlocked.CompareExchange<TEncoding>(FUnicodeEncoding, LEncoding, nil);
    finally
      LEncoding.Free;
    end;
  end;
  Result := FUnicodeEncoding;
end;

[freimatz]

Zitat von Der schöne Günther:

Der Klassendestruktor von TEncoding ruft FreeEncodings() auf, das gibt am Programmende auch das TEncoding.Unicode wieder frei.

Da hast Du recht. Danke. Leider folgt die Freigabe erst nachdem madExcept die Leaks analysiert. Muss da mal weiterschauen was ich da machen kann.

[Uwe Raabe]

Zitat von freimatz:

Muss da mal weiterschauen was ich da machen kann.

Das "No-Leak" mit ThisIsNoLeak anmelden.

[Der schöne Günther]

Oder selbst vorzeitig TEncoding.FreeEncodings() aufrufen.

[Stevie]

Zitat von Schokohase:

Das was man dort sieht ist die übliche Vorgehensweise für die thread-safe und lock-free Erstellung einer (hier globalen) Instanz.

Mit TInterlocked könnte man das noch etwas übersichtlicher gestalten

Jo, was leider a) nicht vernünftig geinlined wird und b) unter ARC zu unnötigem rumgerefcountere führt.

[Schokohase]

Zitat von Stevie:

Jo, was leider a) nicht vernünftig geinlined wird und b) unter ARC zu unnötigem rumgerefcountere führt.

Ich sagte ja auch übersichtlicher (= besser lesbar), was aber nicht immer zwangsläufig technisch gesehen besser sein muss.

Wenn ich mir den Code so ansehe, dann denke ich mir, ob der nicht eher so lauten müsste:

Delphi-Quellcode:

class function TEncoding.GetUnicode: TEncoding;
var
  LEncoding: TEncoding;
begin
  if FUnicodeEncoding = nil then
  begin
    LEncoding := TUnicodeEncoding.Create;
    if AtomicCmpExchange(Pointer(FUnicodeEncoding), Pointer(LEncoding), nil) <> nil then
      LEncoding.Free
    else begin
{$IFDEF AUTOREFCOUNT}
      FUnicodeEncoding.__ObjAddRef;
{$ENDIF AUTOREFCOUNT}
    end;
  end;
  Result := FUnicodeEncoding;
end;

Es wird zwar relativ selten vorkommen, dass mehrere Threads gleichzeitig so eine Encoding Instanz haben möchten und diese noch nicht initialisiert ist, aber wenn man schon damit anfängt, dann sollte man es doch auch richtig machen.

Nehmen wir als 4 Threads, die alle auf die Situation treffen FUnicodeEncoding = nil , alle 4 erzeugen eine Instanz und versuchen diese per AtomicCmpExchange an dem Mann zu bringen, wobei hier gesichert nur einer gewinnt.

Allerdings wird nun trotzdem 4 mal FUnicodeEncoding.__ObjAddRef; ausgeführt und das würde zu einem MemLeak führen, denn am Ende werden 3 Referenzen übrig bleiben.

Ja nichts was einen jetzt so richtig nervös machen kann, aber Leak ist Leak (oder ich habe einfach nur einen Knick in der Optik).

PS: Das wäre mit TInterlocked.CompareExchange nicht passiert

[himitsu]

hmmmm...,

wenn die Referenz übertragen wurde, dann gibt es nun eine Referenz mehr ... konnte nicht automatisch gezählt werden, weil
AtomicCmpExchange durch die "Pointer" die Referenzzählung umgeht.

Wurde es nicht übertragen, weil in der Zwischenzeit ein anderer Thread schneller war, dann bleiben die Referenzen, aber das nun unnötige neue TEncoding muß weg.

Also entweder-oder, aber nicht immer, das AddRef.



Könntest Recht haben.

[Stevie]

https://quality.embarcadero.com/browse/RSP-19796