Best practice to do nested TRY / FINALLY statement

Question

Hi What is the best way to do nested try & finally statements in delphi?

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    cds3  : TClientDataSet;
    cds4  : TClientDataSet;
begin
  cds1      := TClientDataSet.Create(application );
  try
    cds2      := TClientDataSet.Create(application );
    try
      cds3      := TClientDataSet.Create(application );
      try
        cds4      := TClientDataSet.Create(application );
        try
        ///////////////////////////////////////////////////////////////////////
        ///      DO WHAT NEEDS TO BE DONE
        ///////////////////////////////////////////////////////////////////////
        finally
          cds4.free;
        end;

      finally
        cds3.free;
      end;
    finally
      cds2.free;
    end;
  finally
    cds1.free;
  end;
end;

Can you Suggest a better way of doing this?

Graffito

First of all: Don't pass a non-nil Owner to Create if you're going to free the objects yourself anyway. This just adds lots of unnecessary overhead. – Indivisible

Aaaaah trip down memory lane. – Asthmatic

6 months late I know - but technically I think this is correct and is the safe way, All the answers suggested have possible leaks or need backup/helper routines. This code will just work with no leaks. – Yodel

imho there is no better way - only the non-nil constructor should not be used. – Dagenham

Answer 1

how about the following:

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    cds3  : TClientDataSet;
    cds4  : TClientDataSet;
begin
  cds1      := Nil;
  cds2      := Nil;
  cds3      := Nil;
  cds4      := Nil;
  try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);
    cds3      := TClientDataSet.Create(nil);
    cds4      := TClientDataSet.Create(nil);
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    freeandnil(cds4);
    freeandnil(cds3);
    freeandnil(cds2);
    freeandnil(Cds1);
  end;
end;

This keeps it compact, and only attempts to free the instances which were created. There really is no need to perform the nesting since ANY failure will result in dropping to the finally and performing all of the cleanup in the example you provided.

Personally I try not to nest within the same method... with the exception being a try/try/except/finally scenario. If I find myself needing to nest, then to me that is a great time to think refactoring into another method call.

EDIT Cleaned up a bit thanks to the comments by mghie and utku.

EDIT changed the object creation to not reference application, as its not necessary in this example.

Positively

You need to initialize cds2, cds3 and cds4 with nil, otherwise you'll get AVs when freeing the (invalid) objects if the creation of cds1 failed. Only strings and interface pointers can be assumed to be empty when used as stack variables. – Donaugh

You don't need "if Assigned(cdsX)" checks, FWIW. – Thisbee

Please do also note that this code is vulnerable to memory leaks if TClientDataSet.Destroy() raises exceptions. See my own answer for code that can be written to be safe against that as well. – Donaugh

That's really terrible! It's pretty hard to get further away from best practices: – Indivisible

1. the Create must always be placed before the try/finally-free in case of exceptions in the constructor - initializing to nil to prevent this is but an ugly hack. 2. you either pass a non-nil Owner to Create or free the object yourself - never both! – Indivisible

@Oliver: Again, passing a non-nil owner upon creation of components does not prevent manual deletion - the VCL handles that case without any problems. For proof see TComponent.Destroy() and TComponent.Notification() sources. – Donaugh

@Oliver the point was if an exception is raised in one of the create, to automatically call the frees of any of the sub objects and eliminate the nesting. – Positively

@mghie: It does indeed not prevent it so it's not actually "wrong" to do it but the whole Owner-management mechanism does add a lot of completely unnecessary overhead. – Indivisible

Only thing I'd probably change is move 1 of the create out of the Try...Finally. Not that it makes much of a difference, but it still a little bit more optimal. As for possible exceptions in the destructors. The VCL itself hardly bother with that. I just checked TForm.Destroy (D2010 flavor) and it doesn't bother wrapping every free in a try..except. So, as far as the VCL seems concerned, an exception in a destructor seems to be considered a critical failure. – Guthrey

I deal with this using my own InitialiseNil routine which takes multiple parameters, and a FreeAndNil routine that also takes multiple parameters. It makes the above code read a little better. – Selfpity

This is not good. If an exception occurs in one of the first three frees you have a memory leak. – Perlie

@DavidHeffernan could you please tell more about these InitializeNil / FreeAndNil routines? The only solution I came with is InitializeNil (objs: array of PObject) and call it with InitializeNil([@obj1,@obj2,...]), but this doesn't look good. – Hibbard

@Yuri: simply have several overloaded versions of InitializeNil, one with one parameter, one with two, etc. You'll probably not need more than three or four parameters anyway. – Doctrinal

Answer 2

I'd use something like this:

var
  Safe: IObjectSafe;
  cds1 : TClientDataSet;
  cds2 : TClientDataSet;
  cds3 : TClientDataSet;
  cds4 : TClientDataSet;
begin
  Safe := ObjectSafe;
  cds1 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds2 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds3 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds4 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  ///////////////////////////////////////////////////////////////////////
  ///      DO WHAT NEEDS TO BE DONE
  ///////////////////////////////////////////////////////////////////////

  // if Safe goes out of scope it will be freed and in turn free all guarded objects
end;

For the implementation of the interface see this article, but you can easily create something similar yourself.

EDIT:

I just noticed that in the linked article Guard() is a procedure. In my own code I have overloaded Guard() functions that return TObject, above sample code assumes something similar. Of course with generics much better code is now possible...

EDIT 2:

If you wonder why try ... finally is completely removed in my code: It's impossible to remove the nested blocks without introducing the possibility of memory leaks (when destructors raise exceptions) or access violations. Therefore it's best to use a helper class, and let the reference counting of interfaces take over completely. The helper class can free all objects it guards, even if some of the destructors raise exceptions.

Donaugh

Nice implementation, +1 for use, but it eliminates the try finally completely which might be good enough for this example. – Positively

I see try ... finally as little more than a crutch, necessary because RAII is not possible in Delphi without interfaces. If Delphi had stack-allocated objects there would not be much need for try ... finally. Still, it could be used together with the SafeGuard interface. – Donaugh

@mghie: Except for cases when the destructor closes the handle or frees some other resources, which is quite often. – Urogenital

@himself: I do not follow. Destructors should never throw exceptions, as the VCL component ownership management isn't exception-safe. So if all the destructors will be called, what do you mean with "except for cases when..."? – Donaugh

@mghie: If they will be called, then yeah. Thought you were talking about just unwinding stack "in case of anything". On a side note, destructors cannot "never throw exceptions". You don't always know when an exception will be thrown, it's an exception after all. – Urogenital

@himself: If you follow the link to the article you will see that destructors will be called. And while you are right that destructors can throw exceptions they better not do it (at least in TComponent descendants that are used with the VCL lifetime management). Memory leaks and/or crashes are the inevitable consequence. – Donaugh

Answer 3

There's another variation of the code without nested try ... finally that just occurred to me. If you don't create the components with the AOwner parameter of the constructor set to nil, then you can simply make use of the lifetime management that the VCL gives you for free:

var
  cds1: TClientDataSet;
  cds2: TClientDataSet;
  cds3: TClientDataSet;
  cds4: TClientDataSet;
begin
  cds1 := TClientDataSet.Create(nil);
  try
    // let cds1 own the other components so they need not be freed manually
    cds2 := TClientDataSet.Create(cds1);
    cds3 := TClientDataSet.Create(cds1);
    cds4 := TClientDataSet.Create(cds1);

    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////

  finally
    cds1.Free;
  end;
end;

I'm a big believer in small code (if it's not too obfuscated).

Donaugh

Not exactly true. It doesn't really matter whether a component I free manually has an owner or not - the destruction of the object triggers Notification() on the owner, so it removes the sender from its list of owned objects. But I agree, passing nil as AOwner works just as well. – Donaugh

Agreed on the second comment though. Not freeing cds1 (i.e. let Application do it on application shutdown) is little better than a memory leak. – Donaugh

Nice, but the IObjectSafe is better IMO, because it handles TObjects as well as TComponents. – Renewal

@Roddy: I totally agree. There's also a lot more that can be done with such interfaces, I have started to use them all the time (for example for setting and resetting busy cursors, locking and unlocking synchronization objects, temporarily disabling updates and lots of other things). Great stuff. – Donaugh

Answer 4

If you want to go this (IMO) ugly route (group handling with initialization to nil to know if freeing is needed), you at least MUST guarantee that you won't let an exception in one of the destructor prevent from freeing the rest of your objects.
Something like:

function SafeFreeAndNil(AnObject: TObject): Boolean;
begin
  try
    FreeAndNil(AnObject);
    Result :=  True;
  except
    Result := False;
  end;
end;

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    IsOK1 : Boolean;
    IsOK2 : Boolean;
begin
  cds1      := Nil;
  cds2      := Nil; 
 try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);    
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    IsOk2 := SafeFreeAndNil(cds2);    // an error in freeing cds2 won't stop execution
    IsOK1 := SafeFreeAndNil(Cds1);
    if not(IsOk1 and IsOk2) then
      raise EWhatever....
  end;
end;

Hannis

Answer 5

There is a good video on exceptions in constructors & destructors

It shows some nice examples such as:

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
begin
  cds1      := Nil;
  cds2      := Nil; 
 try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);    
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    freeandnil(cds2);    //// what has if there in an error in the destructor of cds2
    freeandnil(Cds1);
  end;
end;

What has if there in an error in the destructor of cds2

Cds1 will not be Destroyed

EDIT

Another good resource is :

Jim McKeeth excellent video on Delayed Exception Handling in code range III were he talks about problems in handling exceptions in the finally block.

Graffito

I commented on that issue already on the answer you accepted. If you want to get rid of the nested finally blocks, use a helper object which frees all guarded objects. This can then be done in an exception-safe way. Do note that TComponent.DestroyObjects() has the same issue! – Donaugh

I just tried, and indeed raising an exception in the destructor of a component owned by a secondary form does lead to access violations when the app is shut down. So it's probably best to try to eliminate all possibilities for exceptions in destructors. – Donaugh

@mghie: Check out the Delayed Exception Handling he linked to. I wrote it specifically to handle exceptions in destructors, since when the code is 3rd Party you can't always change it. – Bunny

@Jim: True, and the VCL is no exception, it's 3rd Party you can't change. I don't know about recent VCL version, but since the VCL ownership management has historically never been exception-safe it wasn't a good idea to have component destructors throw exceptions. If components were owned by a form and freed by the VCL it would cause memory leaks and/or crashes. Your code doesn't help with owned components either. – Donaugh

Answer 6

@mghie: Delphi has got stack allocated objects:

type
  TMyObject = object
  private
    FSomeField: PInteger;
  public
    constructor Init;
    destructor Done; override;
  end;

constructor TMyObject.Init;
begin
  inherited Init;
  New(FSomeField);
end;

destructor TMyObject.Done;
begin
  Dispose(FSomeField);
  inherited Done;
end;

var
  MyObject: TMyObject;

begin
  MyObject.Init;
  /// ...
end;

Unfortunately, as the above example shows: Stack allocated objects do not prevent memory leaks.

So this would still require a call to the destructor like this:

var
  MyObject: TMyObject;

begin
  MyObject.Init;
  try
    /// ...
  finally
    MyObject.Done;
  end;
end;

OK, I admit it, this is very nearly off topic, but I thought it might be interesting in this context since stack allocated objects were mentioned as a solution (which they are not if there is no automatic destructor call).

Orris

Sorry for being not quite clear. The automatic destructor call is exactly what I'm after, whether the instance is stack-allocated and goes out of scope, or whether it is contained in another object and the destructor of contained objects is called when parent is destroyed. RAII as C++ does it... – Donaugh

Do not use old-style objects. They do not work well with inheritance, and they do not support any Delphi types introduced in the last decade, such as long strings, interfaces, or dynamic arrays. – Baseman

@Rob: What do you mean by old stlye objects? TObject? What is the best alternative? – Hedgcock

@Luke: An old style object is created with TMyObject = object instead of TMyObject = class or even TMyObject = class(TObject). Old style objects have been deprecated for a long time, and are only there for backwards compatibility. – Donaugh