Should I put a try-finally block after every Object.Create?

Question

I have a general question about best practice in OO Delphi. Currently, I put try-finally blocks anywhere I create an object to free that object after usage (to avoid memory leaks). E.g.:

aObject := TObject.Create;
try
  aOBject.AProcedure();
  ...
finally
  aObject.Free;
end;

instead of:

aObject := TObject.Create;
aObject.AProcedure();
..
aObject.Free;

Do you think it is good practice, or too much overhead? And what about the performance?

Bath

Answer 1

In my opinion, there is only one reason an objects construction should not be followed by (or "in" as Mason pointed out) a try / finally block.

1. If an objects' lifetime is being managed by another object.

This management can take three forms:

1. An object's reference has a scope beyond the local block and is freed elsewhere - like a field member freed in the destructor.
2. An object immediately added to a list which is in charge of freeing the object later.
3. An object with an associated lifetime manager, like how you pass the owner to a VCL control on construction.

With #1, when the reference has a broader scope, the reference should be set to nil immediately if it isn't constructed right away. That way when it is checked for reference you know you have an accurate reading. This is most common with member objects that are constructed as part of a larger class, and then cleaned up when the parent object is destroyed.

With #2, when an object is added to a list, you want to use a try-except block (one of the few times I use one) just in case an exception occurs after the object is constructed and before it is added to the managing list. Ideally the first line after the construction is adding it to the list, or the list is actually a factory class that gives you an object already added to the list.

With #3, when an object has another lifetime manager, you really should make sure having it managed by that manager is the right thing to do. If you are constructing a VCL control, you may be tempted to have the form (or any other control) own it, but that actually puts additional overhead on the construction and destruction. If possible you should explicitly free it, this is especially true if you put the control on once, then you know you will be freeing it in your form's destructor or when it closes. The only time you can't do this is if the control creation is more dynamic.

So yes, it is a best practice to use a lot of try / finally blocks. You should only have a few try / except blocks, and most all of them should trap very specific exception types, and / or re-raise the exception. If you have more try / except than try / finally blocks, then you are doing it wrong.

Rosebud

Very good answer. ANd you included some interesting and salient stuff that the OP didn't think to ask about. Such as the problems with having TOO MANY coarse "try..catch" blocks that effectively kill your ability to manage code failures, and exit from failed code call-stacks cleanly. – Mezuzah

Answer 2

It's definitely best practice to use try-finally.

In the event of an exception being raised, that object will be freed.

As for performance: measure before you optimise.

Favianus

Answer 3

As Frank said, "as for performance: measure before you optimize." Repeating it to emphasize.

Also, if you're creating a bunch of objects in a method, you don't need to use a try..finally block for each of them. That can lead to an ugly indentation mess. create, try, create, try, create, try, do something, finally, free, finally, free, finally, free. Ugh! Instead, you can set the object references to nil at the top of the method, then create them all, do one try block, and free them all in the finally section.

That'll save some overhead and performance, (though you'll probably never notice the difference,) but more importantly it'll make your code cleaner and easier to read while maintaining the same level of safety.

Estrogen

+1 for repetition :) Also, if you're creating so many objects that you have an indentation problem, you have a bigger problem: your method's doing too much. – Favianus

+1 Nested try/finally for every object.create is fugly. 1 per block is usually enough. – Gloriane

If one of the destructors throws an exception the following ones won't get freed. Tidy.. perhaps, but I don't think it is good practice. – Costumier

@sertac: If you have destructors raising exceptions, you've got bigger problems to worry about than memory leaks! If you're putting code that could raise an exception inside a destructor, or in fact any code that does anything other than release resources, you're almost certainly doing something very wrong. – Estrogen

@mason: you can't control everything, at least I think so, releasing things requires code and sth. go wrong... I've seen destructors raising exceptions, but I don't recall if it was only my code.. – Costumier

I find that when I'm nesting Try Finally's it's time for a new function. I may write it with nested try blocks initially, but then I cut the code out, I find it to be especially useful because the variables creates inside the outer try block most certainly aren't used by the rest of the procedure and it's just nicer to have the var section closer to where you use the variable. – Stunk

@Mason I have seen 3rd party class destructors raise exceptions. Not to say those destructors were not written wrong, but unless you write all the code you use or call, and never make a mistake, then it can happen, and can be a pain to track down if that exception gets stomped on by another exception. I created a SafeFreeAndNil method a while ago to deal with exactly that scenario. – Rosebud

The problem with raising exceptions in destructors is that there's not really any way to sensibly recover from the problem. – Ailsun

Here's a concrete example for an exception in a destructor; the GlassFrame bug specially crafted for only D2007 (QC# 55210, 61846, 68300) - "raising an exception in 'TCustomForm.constructor' before calling 'inherited' causes AV in the destructor". And this is even not exactly 3rd party code.. – Costumier

Answer 4

To answer part two of your question:
try finally hardly has any overhead.

In fact there are plenty of methods having an implicit try...finally block. For instance just having a function using a local var of any Interface type and assigning that a value has.

--jeroen

Woody

...or a string, for that matter. – Estrogen

I'd meant to imply the low cost of a try/finally, but obviously didn't succeed :) It's a bit like worrying about the performance hit of a procedure call. – Favianus

Answer 5

Last year at Delphi Developer days I saw some of Marco Cantu's private stash of code and he calls try finally every single time he creates anything.

Somebody asked him about it, and he said he tries to do it all the time.

But it's especially a good idea for multi-threaded code when it comes to entering and exiting critical sections, although that's not on the topic, it's a good thing to remember.

Obviously sometimes it's a bit obtrusive and if it's not in the culture of your work environment to be spot-on in your robsutness it may make you look like a goodie-two-shoes. But I think it's a good idea. It's kind of like Delphi's attempt at enforced manual garbage collection.

Stunk

"but it's especially a good idea for multi-threaded code." - it has absolutely no bearing what-so-ever on multi-threaded code. It's essential all the time, never mind being a good idea or better in one circumstance or another. If you find it obtrusive then you should consider something like my AutoFree() approach: deltics.co.nz/blog/?p=391 – Myrmidon

I heartily disagree, although yeah you don't create your threads and free them with try/finally blocks, but you do want to go into your critical sections and out of them with try/finallys. I'll modify my answer. – Stunk

Answer 6

If you're creating the object in a class's constructor and the object will be owned by the enclosing class, you'll want to free it in the owning class's destructor.

I tend to use FreeAndNil() instead of calling Free.

EDIT: But as others have said, you definitely always want to free what you create.

Ebbie

Answer 7

Yes it is always a good idea (essential) if the code that creates the object is responsible for free'ing it. If not, then try/finally isn't appropriate but then again neither is .Free in any case!

However, it can be cumbersome to have this boilerplate code peppering your "business logic", and you might want to consider an approach which has the same guarantee of freeing your objects but which is much cleaner (and has other benefits), such as my own AutoFree() implementation.

With AutoFree() your code could then be written:

aObject := TObject.Create;
AutoFree(@aObject);

aObject.AProcedure();

or alternatively, since the implementation uses a reference to a reference (to enable auto-NIL'ing as well as Free'ing), you can even pre-register your references that you wish to be AutoFree'd to keep such housekeeping declarations away from your business logic and keep the real "meat" of your code as clean as possible (this is especially beneficial when potentially multiple objects need to be Free'd):

AutoFree(@aObject1);
AutoFree(@aObject2);

aObject1 := TObject.Create;
aObject1.AProcedure();

// Later on aObject2 is (or may be) also created
 :

Not shown in my original posting is a later addition to the mechanism to support registering multiple references in a single AutoFree() call, but I'm sure you can figure out the changes needed to support this on your own, if you wish to be able to do this:

AutoFree([@aObject1, @aObject2]);

aObject1 := TObject.Create;
aObject1.AProcedure();

// Later on aObject2 is (or may be) also created
 :

Myrmidon

Answer 8

Even if it is much recommended to do that, I won't always do it.

My rules of using or not using the try/finally:

• The chance for that object to crash and burn.
• Number of times the object is created. If I know that your object is created rarely (not more than 5-6 time in application life time) I can live with a 20KB memory leak - in case it 'dies' without being freed.
• Amount of leaked memory in case the object crashes.
• Code complexity. Try/except is making the code to look really ugly. If there is a 5 lines procedure, I always use try/except.
• Application file span. If your application needs to run for days, then I definitively want to free ANY piece of memory else the leak will accumulate.

The only place where is difficult to make a decision is when you need performance while creating the object thousands of times (in a loop for example). In this case, I don't use try/except if the object is performing simple tasks and there is a small chance to see it crashing.

Leningrad

Completely disagree. That is an anti-pattern, and should only be used in rare cases. – Rosebud

I hope you are not designing operating systems. On my computer, Windows 7 often runs for months. – Boyle

-1 So you really think about all these questions (and probably draw a few wrong conclusions on the way) every time you create an object instead of just making it a habit to use try..finally .. I don't get it ... btw: the topic is try..finally not try..except. – Scholar