20

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?

4
  • 3
    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. Dec 31, 2008 at 12:03
  • Aaaaah trip down memory lane. Feb 5, 2009 at 14:01
  • 2
    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.
    – Despatcher
    Jun 19, 2009 at 21:09
  • 1
    imho there is no better way - only the non-nil constructor should not be used.
    – mjn
    Mar 25, 2010 at 16:40

6 Answers 6

31

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.

13
  • 1
    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.
    – mghie
    Dec 29, 2008 at 17:30
  • 1
    That's really terrible! It's pretty hard to get further away from best practices: Dec 31, 2008 at 11:57
  • 2
    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! Dec 31, 2008 at 11:58
  • 3
    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. Jan 7, 2011 at 16:59
  • 1
    This is not good. If an exception occurs in one of the first three frees you have a memory leak.
    – Pieter B
    Jul 31, 2012 at 14:29
18

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.

6
  • 1
    Nice implementation, +1 for use, but it eliminates the try finally completely which might be good enough for this example.
    – skamradt
    Dec 29, 2008 at 17:42
  • 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.
    – mghie
    Dec 29, 2008 at 17:50
  • @mghie: Except for cases when the destructor closes the handle or frees some other resources, which is quite often.
    – himself
    Oct 20, 2010 at 16:17
  • 1
    @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.
    – himself
    Oct 21, 2010 at 10:44
  • 1
    @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.
    – mghie
    Oct 21, 2010 at 10:56
4

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).

4
  • 1
    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.
    – mghie
    Dec 31, 2008 at 12:33
  • 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.
    – mghie
    Dec 31, 2008 at 12:35
  • Nice, but the IObjectSafe is better IMO, because it handles TObjects as well as TComponents.
    – Roddy
    Jan 6, 2009 at 10:25
  • @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.
    – mghie
    Jan 6, 2009 at 21:04
3

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;
2

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.

4
  • 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!
    – mghie
    Jan 10, 2009 at 11:10
  • 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.
    – mghie
    Jan 10, 2009 at 11:30
  • @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. Oct 28, 2010 at 18:31
  • @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.
    – mghie
    Oct 28, 2010 at 19:47
1

@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).

4
  • 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...
    – mghie
    Dec 29, 2008 at 19:02
  • 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. Dec 30, 2008 at 22:04
  • @Rob: What do you mean by old stlye objects? TObject? What is the best alternative?
    – lukeck
    Jan 2, 2009 at 3:43
  • @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.
    – mghie
    Jan 2, 2009 at 8:39

Your Answer

Reminder: Answers generated by Artificial Intelligence tools are not allowed on Stack Overflow. Learn more

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.