TryGetvalue doesn't work if individual items are freed - memory leaks if they aren't

Question

I discovered that the Win32 compile of my FMX application has a bug!

The problem is simply that the TryGetValue returns unknown myPOi's if I {$define FREE_MYPOI}. If I don't define it (ie. not free each instance of myPOI after it is added to the dictionary), then Eurekalog reports memory leaks when the app exits (my POI hasn't been freed obviously).

Most interesting, is that this problem doesn't occur in the Android version of the code with FREE_MYPOI defined. (thankfully)

Is my way of adding items to the Dictionary correct? (for Android and Win32 FMX)

Here is the core code:

type
  TMyPOI = class(TObject)
  public
    Value: Integer;
    Timestamp: TDateTime;
  end;

...

function TForm2.CreateOrUpdate(username: String; NewTimestamp: TDateTime): String;
var
  poiTimeStamp: TDateTime;
  myPOI: TMyPOI;
  Index: Integer;
begin

  if PoiDict.TryGetValue(username, myPOI) then
  begin
    // existing POI
    Result := InttoStr(myPOI.Value) + ' ' + DateTimeToStr(myPoi.Timestamp);
    poiTimestamp := myPOI.Timestamp;
    // update the Poi's timestamp
    myPOI.Timestamp := NewTimeStamp;
    PoiDict.AddOrSetValue(username, myPOI);
  end
  else
  begin
    // add a new POI
    Index := Random(999);
    Result := IntToStr(Index) + ' ' + DateTimeToStr(NewTimeStamp);
    myPOI := TMyPOI.Create;
{$ifdef FREE_MYPOI}
    try
{$endif}
      myPOI.Value := Index;
      myPOI.Timestamp := NewTimeStamp;
      PoiDict.Add(username, myPOI);
{$ifdef FREE_MYPOI}
    finally
      myPOI.Free;
    end;
{$endif}
  end;

end;

initialization
  PoiDict := TDictionary<string, TMyPOI>.Create;

finalization
  PoiDict.Free;

end.

Addendum: This is not an ARC-specific question. It is a question about managing object references.

No defect in RTL code, your code is broken. With the conditional undefined nothing frees the objects and they leak bit it defined you free them while the dictionary still holds a reference which is even worse. — David Heffernan, Aug 20 '17 at 12:03
I offer a simplification of what you're doing wrong: If you write `TempPOI := TMyPOI.Create; TempPOI.Free; Writeln(TempPOI.Value);` you can ***expect*** that to not work correctly because `TempPOI` is now ***invalid*** because it's ***destroyed***. Conversely if you ***never*** free the object, ***of course*** you have a memory leak. The point is you need to know when to fold 'em; I mean ***correct*** time to destroy the objects. Don't just try random stuff in the hopes something works. Make sure you ***understand*** your code - don't gamble with it! — Disillusioned, Aug 20 '17 at 13:12
Possible duplicate of [Delphi ARC under Firemonkey for Windows](https://stackoverflow.com/questions/44374131/delphi-arc-under-firemonkey-for-windows) — Disillusioned, Aug 20 '17 at 13:15
@nolaspeaker Your question is most certainly is related to ARC and your misunderstanding of it. You make specific reference to how your code reports leaks under Win32 (non-ARC) but doesn't under Android (ARC). And this is all because you're not managing your memory correctly. You could be expecting ARC behaviour in Win32 (Hence why I suggested the poss-dup.) But of course your mistakes may lie in a fundamental lack of understanding about basic resource management. (Hence why it's only ***possible*** duplicate.) — Disillusioned, Aug 20 '17 at 23:27
@Craig Young. That's why I asked the question! (You know there's something wrong, but you don't have enough experience to say what it is exactly) The answer from Dalija has certainly enlightened me and I won't the same mistake again. — Freddie Bell, Aug 21 '17 at 07:11
@nolaspeaker But you were arguing that it's not a duplicate; yet clearly the poss-dup has answers that may resolve confusion (for you _or someone else with a similar question_ ). ... Let's not keep going in silly circles. ;) — Disillusioned, Aug 21 '17 at 10:05

Green_Wizard · Answer 1 · 2017-08-20T11:46:55.323

You are freeing object that stored/added to dictionary. Don't free it! Or, better, use interfaces and forget about manually freeing objects.

P. s. why it's happens? - because objects are stored NOT by value, it stored by pointer/reference.

P.P.S. I hope you use TObjectDictionary and values not leaking.

I read again your question and you definitely not using TObjectDictionary or manually cleaning up dictionary values, which cause memory leak. You have to free object when dictionary cleaning/free, not after Add.

Dalija Prasnikar · Accepted Answer · 2017-08-21T17:14:19.897

First, don't ever Free any object you just created and added to any kind of collection. You should transfer ownership over that object to that collection. Following code is inherently broken on Windows - because you are creating dangling references inside dictionary - they will blow on you sooner or later.

    myPOI := TMyPOI.Create;
{$ifdef FREE_MYPOI}
    try
{$endif}
      myPOI.Value := Index;
      myPOI.Timestamp := NewTimeStamp;
      PoiDict.Add(username, myPOI);
{$ifdef FREE_MYPOI}
    finally
      myPOI.Free;
    end;
{$endif}

You should use TObjectDictionary that takes ownership on the values. This will work without the leaks on all platforms.

 PoiDict := TObjectDictionary<string, TMyPOI>.Create([doOwnsValues]);

Next thing, myPOI is an object, so you don't have to add it again with changed timestamp, you can just change timestamp directly - PoiDict.TryGetValue(username, myPOI) will just give you the reference it will not create copy of the object.

Corrected code would look like:

function TForm2.CreateOrUpdate(username: String; NewTimestamp: TDateTime): String;
var
  poiTimeStamp: TDateTime;
  myPOI: TMyPOI;
  Index: Integer;
begin

  if PoiDict.TryGetValue(username, myPOI) then
  begin
    // existing POI
    Result := InttoStr(myPOI.Value) + ' ' + DateTimeToStr(myPoi.Timestamp);
    poiTimestamp := myPOI.Timestamp;
    // update the Poi's timestamp - this will update object in dictionary 
    myPOI.Timestamp := NewTimeStamp;
  end
  else
  begin
    // add a new POI
    Index := Random(999);
    Result := IntToStr(Index) + ' ' + DateTimeToStr(NewTimeStamp);
    myPOI := TMyPOI.Create;
    try
      myPOI.Value := Index;
      myPOI.Timestamp := NewTimeStamp;
      PoiDict.Add(username, myPOI);
    except
      myPOI.Free;
      raise;
    end;
  end;

end;

initialization
  PoiDict := TObjectDictionary<string, TMyPOI>.Create([doOwnsValues]);

finalization
  PoiDict.Free;

end.

Thank you! Of course, my lack of experience is showing, and your answer fills in the gaps in my knowledge. I don' think this is a specific ARC question, it's a question of understanding how object references work. — Freddie Bell, Aug 20 '17 at 13:43
At the very least, on non-ARC systems, the `try..finally` should be replaced with `try..except` to call `myPOI.Free() ` if `PoiDict.Add()` raises an exception. — Remy Lebeau, Aug 21 '17 at 02:09
@nolaspeaker adding a value to any container requires allocating memory. That can fail, albeit rare — Remy Lebeau, Aug 21 '17 at 07:42
@RemyLebeau You are correct, `Add` can fail and in that case `myPOI` would leak. Code corrected. — Dalija Prasnikar, Aug 21 '17 at 17:16

TryGetvalue doesn't work if individual items are freed - memory leaks if they aren't

2 Answers2