CODEWITHSUNDEEP
classdelphimemory-leakststringlist
REALSOFO
REALSOFO

Reputation: 862

Delphi: Memory leak when creating a TStringList inside a class

I have this pieces of code

TSql = class
  private
    FConnString: TStringList;
  public
    property ConnString: TStringList read FConnString write FConnString;
    constructor Create;
    destructor Destroy;
  end;

var 
  Sql: TSql;

...

implementation

{$R *.dfm}

constructor TSql.Create;
begin
  //inherited Create;
  FConnString:=TStringList.Create;
end;

destructor TSql.Destroy;
begin
  FConnString.Free;
  //inherited Destroy;
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  Sql.Create;
  Sql.ConnString.Add('something');
  showmessage(Sql.ConnString.Text);
  Sql.Destroy;
end;

Why when creating the FConnString is creating a memory leak, after a press the button?

.................................. .................................. .................................. ..................................

Upvotes: 0

Views: 1981

Answers (4)

Disillusioned
Disillusioned

Reputation: 14842

EDIT The actual problem

The original code in the question was as follows:

procedure TForm1.Button1Click(Sender: TObject);
begin
  Sql.Create;
  Sql.ConnString.Add('something');
  showmessage(Sql.ConnString.Text);
  Sql.Destroy;
end;

The problem line was Sql.Create; which should have been Sql := TSql.Create;. The reason this causes a memory leak is as follows:

  • Sql.Create; is called from a nil reference.
  • This calls TStringList.Create; and attempts to assigned the result to FConnString.
  • Because Sql is a nil reference, this triggers an Access Violation.
  • The problem is there is no way to destroy the TStringList instance that was created.

Additional problems in original answer

Your destructor is virtual, you're not overriding.
You're not calling your inherited Destructor.

TSql = class
  private
    FConnString: TStringList;
  public
    property ConnString: TStringList read FConnString write FConnString;
    constructor Create;
    destructor Destroy; override; //Correction #1
  end;

destructor TSql.Destroy;
begin
  FConnString.Free;
  inherited Destroy; //Correction #2
end;

EDIT

A few general tips:

  1. I applaud you for using composition (making FConnString a member instead of inheriting from TStringList). However, by exposing it publicly, you would lose many of the benefits. Specifically you would be exposed to Law of Demeter violations. I'm not saying never do it. But be aware that you can create a maintenance problem down the line if a large amount of client code accesses ConnString directly.

  2. Declaring FConnString: TStringList; violates the principle of Program to an interface, not an implementation. TStringList is a specific implementation of TStrings and this declaration prevents use of other subclasses of TStrings. This is more of a problem combined with #1: if in a few years time you find and want to switch to a different/better subclass implementation of TStrings client code that is binding to TStringList now creates much more work and risk. Basically the preferred aproach can be summed up as:

    • Declare the variable as the abtract base type.
    • Create the instance as the specifically chosen implementation subclass.
    • Let polymorphism ensure subclass behaviour is applied correctly in overridden methods.

Again this is a general guideline. If you specifically need access to properties/methods that were added at the TStringList level of the hierarchy, then you have to bind to that class. But if you don't need it, don't do it.

Upvotes: 9

Allen Bauer
Allen Bauer

Reputation: 16862

There are two things I see. The first of which was already covered by the other comments and answer regarding the lack of "override" on the destructor.

The second issue is the property declaration itself. In general, you should never declare a property that references an object field in the "write" clause. The reason is that assigning to that property will "leak" the existing instance in that field. Use a method for the "write" clause of the property declaration:

property ConnString: TStringList read FConnString write SetConnString;
...
procedure TSql.SetConnString(Value: TStringList);
begin
  FConnString.Assign(Value);
end;

Also notice that this method does not overwrite the FConnString field either. It merely copies the "value" or the "content" of the Value TStringList into the FConnString instance. In this manner the TSql instance is in complete and total control over the lifetime of that field. It is the responsibility of the code assigning that property to control the lifetime of the Value TStringlist.

Upvotes: 11

REALSOFO
REALSOFO

Reputation: 862

The object is not created correctly. It must be:

Sql:=TSql.Create;

From here is coming the memory leak.

Upvotes: 0

Remy Lebeau
Remy Lebeau

Reputation: 597961

You forgot to declare Destroy() with the override specifier, so TSql.Destroy is not actually being called when the object is destroyed.

destructor Destroy; override;

Upvotes: 1

Related Questions