7

In Delphi 5, I've currently written code that calls Free on multiple variables in a finally block, e.g.

...
finally
    a.Free;
    b.Free;
    c.Free;
end;

This code assumes that Free can never raise, since if, for example, a.Free raised, the memory for b and c would be leaked. Is this assumption justified?

Stuart Golodetz
  • 20,238
  • 4
  • 51
  • 80

5 Answers5

11

The Free method itself does not explicitly raise an exception, but it calls the virtual destructor Destroy which certainly could raise an exception.

So if you want to be sure that all your objects are destroyed, even if one of the destructors raises an exception you end up with code like this:

a := TMyObject.Create;
try
  b := TMyObject.Create;
  try
    ...
  finally
    b.Free;
  end;
finally
  a.Free;
end;

Having said that, it should be a design principle that you do not raise exceptions in a destructor. So, in my view it's perfectly reasonable to take the viewpoint that if an exception is raised in destructor, then your program is pretty much hosed. Leaking objects at that point is not something to worry about. If your destructor has raised an exception then you are probably already leaking because that destructor did not run to completion.

So in my view it can be perfectly reasonable to group together some calls to Free and of course you avoid deeply nested try/finally which is something worth striving for.

If you want just one try/finally then remember to write the code like this:

a := nil;
b := nil;
try
  a := TMyObject.Create;
  b := TMyObject.Create;
  ...
finally
  b.Free;
  a.Free;
end;

In my own code base I have some helper methods that make this cleaner. Then the code can look like this:

InitialiseNil(a, b);
try
  a := TMyObject.Create;
  b := TMyObject.Create;
  ...
finally
  FreeAndNil(b, a);
end;

I have given my FreeAndNil the same name as the function in SysUtils which on first glance may seem odd, but it is safe and benign to do so. Naturally these helpers come into their own when you have even more than two objects.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
3

Depends on what's happening in the destructor.

Ondrej Kelle
  • 36,941
  • 2
  • 65
  • 128
  • Totally correct, I deleted my wrong anser. Did not take destructors into account. That's why destructors should not throw exceptions. – jpfollenius Feb 08 '12 at 10:11
2

There could be 2 things that can cause SomeObj.Free to raise an exception:

  1. Unhandled exception in destructor of the SomeObj instance of class or on of its ancestors.
  2. Invalid class reference due to uninitialized variable SomeObj.

In your case if a.Free raises an exception for any of the above reasons, there would be a memory leak for object b and c and maybe some leak inside object a because of unhandled exception in destructor.

Vahid Nasehi
  • 455
  • 5
  • 9
1

if your a.free raise an exception, a(depending on how much the destructor has freed up from the the a's object fields), b and c objects will be leaks because the execution will be interrupted. Anyway, something is wrong in your destructor, if it raises an error. so, you should protect the code with try..finally blocks, but IMHO you should verify that destructors don't give you in any circumstance errors.

RBA
  • 12,337
  • 16
  • 79
  • 126
  • 2
    Stuart's assumption is "Free can't raise an exception", and it clearly can. – Frank Shearar Feb 08 '12 at 10:24
  • 1
    Don't forget, the `a` object will also leak if its destructor raises an exception. –  Feb 08 '12 at 10:26
  • 1
    I actually did mean `a` will always leak, because the memory for `a` itself definitely won't be freed, but you're right to point out that some of its subobjects may be. –  Feb 08 '12 at 10:33
  • Thanks all. I'm coming at this from a C++ background, where destructors which can throw are a serious no-no (since throwing whilst the stack is being unwound as a result of a previous exception calls `std::terminate`), so I made the (what turns out to be invalid) assumption that something similar was the case in Delphi. – Stuart Golodetz Feb 08 '12 at 10:50
  • 1
    @StuartGolodetz The application doesn't terminate on Delphi double exceptions, but that doesn't mean raising exceptions from a destructor is a good idea. You should never do it in Delphi either. :) –  Feb 08 '12 at 11:08
0

Of course FREE can emit exceptions - so yes, you will leak memory in your code if A.FREE emits an exception, B.FREE and C.FREE will not be called.

The question is, do you want to handle the exceptions or let them happen? Its going to depend on what your code is going to be for, are other devs going to be using it (for example). To prevent any memory leakage you should nest the try..finally sections;

a:=tobject.create;
try
  b:=tobject.create;
  try
    c:=tobject.create;

    ...

  finally
    c.free;
  end;
finally
  b.free;
end;
a.free;

Sort of thing. Its a question of what your code is actually doing as to if you should also wrap the A.FREE in a try..finally section too, although I'm guessing you probably should.

Paul Foster
  • 85
  • 1
  • 3
    Your `a.free;` is outside of any `try`..`finally`, and you try to free `b` if `b:=tobject.create;` raises an exception (same with `c`). –  Feb 08 '12 at 10:23
  • 1
    Besides, if `c.free` raises an exception, you still have a leak because you haven't actually freed `c`. –  Feb 08 '12 at 10:25
  • Its debatable that this is good coding style anyway - but I did say that you should probably try..finally the a.free too; I was only demonstrating how you could do it :-) – Paul Foster Feb 08 '12 at 11:00
  • 1
    In your example, you should definitely wrap the `a.free;` in a `try`..`finally` too. If you're absolutely 100% sure that the code can never raise an exception, you don't need it, but you then don't need any of the `try`..`finally`s that you did include. –  Feb 08 '12 at 11:02