CODEWITHSUNDEEP
delphimemorydelphi-7
Maulik Shah
Maulik Shah

Reputation: 402

Free memory of Object assigned to multiple StringList

I have an object which is assigned to multiple TStringList objects.

Releasing memory for first TStringList works fine. But when i call Object.Free for the second TStringList, it results in Access Violation.

This is how the object is assigned to the 2 TStringList

     SL1.AddObject(S, Person);
     if (Sl2<> nil) and (Var1 <> '')
       then Sl2.AddObject(Var1,TObject(Person));

This is how memory is released.

SL1

Method:ReleaseSL1

For J := SL1.Count - 1 downto 0  do
  begin
     if (SL1.Objects[J]) <> nil then
       begin
         PersonObj:= TPerson(SL1.Objects[J]);
         PersonObj.Free;
         SL1.Objects[J] := nil;
       end;
  end;

SL2

Method:ReleaseSL2

  for I := 0 to SL2.Count-1 do
    if SL2.Objects[I] <> nil then
      begin
        PersonObj := TPerson(SL2.Objects[I]);
        PersonObj .Free;
        SL2.Objects[I] := nil;
      end;
  SL2.Clear;

Here, when PersonObj.Free is executed in method ReleaseSL2, it results in Access Violation

Is there any way, apart from restructuring of code, that I can avoid this Access Violation?

Note: There are many more objects assigned to both SL1 and SL2 and the 2 TStringList objects are not identical

Upvotes: 1

Views: 885

Answers (3)

Remy Lebeau
Remy Lebeau

Reputation: 597111

Is there any way, apart from restructuring of code, that I can avoid this Access Violation?

No. A restructure is required. You need to clearly define who owns the TPerson objects, and who merely refers to them. In your example, SL1 can own the objects and SL2 should refer to them. Free the objects only from the list that owns them, and remove any references from the list that refers to them without freeing them again.

SL1.AddObject(S, Person); // <--assumed ownership
if (Sl2 <> nil) and (Var1 <> '') then
  Sl2.AddObject(Var1, Person); // <-- reference only


For J := SL1.Count - 1 downto 0  do
begin
  if (SL1.Objects[J]) <> nil then
  begin
    TPerson(SL1.Objects[J]).Free;
    SL1.Objects[J] := nil;
  end;
end;

//...

SL2.Clear;

Otherwise, you need to use a third list that owns the objects, and then have both SL1 and SL2 merely refer to them:

Objects.Add(Person); // <-- asssumed ownership
SL1.AddObject(S, Person); // <-- reference only
if (Sl2 <> nil) and (Var1 <> '') then
  Sl2.AddObject(Var1, Person); // <-- reference only


For J := SL1.Count - 1 downto 0  do
  SL1.Objects[J] := nil;

//...

SL2.Clear;

//...

For J := 0 to Objects.Count - 1 do
  TPerson(Objects[J]).Free;
Objects.Clear;

If you use a TObjectList or TObjectList<TPerson> for the third list, you can set their OwnsObjects property to True to auto-manage the lifetime of the TPerson objects for you.

FYI, on modern Delphi versions, TStringList also has an OwnsObjects property, but I would not suggest using it in this situation.

Upvotes: 1

Ken White
Ken White

Reputation: 125728

You're freeing the object and only nilling one of the two references to that object (the one in SL1).

Consider the following test application:

program Project1;

{$APPTYPE CONSOLE}

uses
  SysUtils;

type
  TTest = class(TObject);

var
  A, B: TTest;

begin
  A := TTest.Create;
  B := A;
  WriteLn(Format('Assigned A: %s  B: %s',
         [BoolToStr(Assigned(A), True), BoolToStr(Assigned(B), True)]));

  FreeAndNil(A);
  WriteLn(Format('Assigned A: %s  B: %s',
         [BoolToStr(Assigned(A), True), BoolToStr(Assigned(B), True)]));
  ReadLn;
end.

The output it produces:

Assigned A: True  B: True
Assigned A: False  B: True

Note that, even though you've called FreeAndNil on A, it doesn't automagically set the value of B to nil as well. The compiler has no way of knowing that you're finished with the other reference.

The solution to your particular issue is simple: when freeing the object in SL1, see if it exists in SL2 and nil it at the same time. (You also don't need the local variable PersonObj. If you've properly overridden the inherited methods, the proper Free will be called for you.)

var
  RefIdx: Integer;

For J := SL1.Count - 1 downto 0 do
begin
  if SL1.Objects[J] <> nil then
  begin
    RefIdx := SL2.IndexOfObject(SL1.Objects[J]);
    if RefIdx > -1 then
      SL2.Objects[RefIdx] := nil;

    SL1.Objects[J].Free;
    SL1.Objects[J] := nil;
  end;
end;

Upvotes: 1

JimmyB
JimmyB

Reputation: 12610

To make sure you don't try to free an object tiwce, you can use somehing like this:

For J := SL1.Count - 1 downto 0  do
  begin
     if (SL1.Objects[J]) <> nil then
       begin
         PersonObj:= TPerson(SL1.Objects[J]);

         // Make sure this object is not kept in SL2 any more:
         SL2.remove(PersonObj);

         PersonObj.Free;
         SL1.Objects[J] := nil;
       end;
  end;



  for I := 0 to SL2.Count-1 do
    if SL2.Objects[I] <> nil then
      begin
        PersonObj := TPerson(SL2.Objects[I]);
        PersonObj .Free;

        // Make sure this object is not kept in SL1 any more:
        SL1.remove(PersonObj);

        SL2.Objects[I] := nil;
      end;
  SL2.Clear;

One note: You only need to remove the object from the other list if that list is not already cleared.

Upvotes: 1

Related Questions