Another case against FreeAndNil
Posted by jpluimers on 2017/12/21
I started this post as “A case against FreeAndNil” but soon found out about [WayBack/Archive.is] A case against FreeAndNil – Community Blogs – Embarcadero Community.
Recently I was refactoring a bunch of code that was classed based to become interface based.
The old code had a lot of references to classes like this:
FMyField: TMyClass;
The compiler had detected those as compiler errors so now they were like this:
FMyField: IMyInterface;
But the compiler would still happily compile code (for instance in destructors) containing:
FreeAndNil(FMyField);
The only way that FreeAndNil can be implemented is with an untyped var parameter:
procedure FreeAndNil(var Obj); var Temp: TObject; begin Temp := TObject(Obj); Pointer(Obj) := nil; Temp.Free; end;
This var construct makes it typeless: the method dutifully accepts anything even something that isn’t a TObject. And there is hardly anyw any (cheap) way to detect it’s actually a TObject (as it likekly won’t have RTTI).
What happens when you call it with an interface as parameter is that the hard cast will execute a “Free” method that might not be a method at all. Or is a method that does something totally different, expects more parameters (hello stack-underflow!) or has other harmfull side effects.
So contrary to the past – where most people would prefer FreeAndNil – I’m back to the below code in class based situations:
FMyField.Free(); FMyField := nil;
This looks like a strange but it has the visual clue that both are being executed and belong together.
Edit: the above is why I wrote a generic
FreeAndNilinside a helper that limits itself toTObjectreferences. The gist is below. This is the declaration:
class procedure FreeAndNil<T: class>(var Value: T); static;
You can call it like this:
var Instance: TInterfacedObject; Reference: IInterface; begin Instance := TInterfacedObject.Create(); Reference := TInterfacedObject.Create(); TObjectHelper.FreeAndNil(Instance); // compiles and is type-safe TObjectHelper.FreeAndNil(Reference); // fails to compile end.
Some future enhancements of the typesafe FreeAndNil<T>:
- The
nilcould be a totally different value likePointer(-1)abstracted away into aconstlikeUnusedPointerto better detect use-after free. A similar construct (with the drawback of using an untypedvarparameter) is suggested at [WayBack] Don’t abuse FreeAndNil anymore – DelphiTools. - Some hinting comments to a code formatter will not break the lines apart. I need to look into [WayBack] How to turn off source formatter in Delphi 2010 for sections of code? – Stack Overflow more closely later.
Blast from the past which is how I used to do it:
- [WayBack] Why should you always use FreeAndNil instead of Free
- [WayBack] delphi – Which is preferable: Free or FreeAndNil? – Stack Overflow
–jeroen
PS: Seems Lars Fosdal missed it, so I reminded him at [WayBack] There was a bug in the (yet unreleased) GExperts code that caused an access violation every time the Delphi IDE was closed. I have just found it, but bo… – Thomas Mueller (dummzeuch) – Google+
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| unit ObjectHelperUnit; | |
| interface | |
| type | |
| TObjectHelper = record | |
| class function Cast<T: class>(const aValue: TObject): T; static; | |
| class procedure FreeAndNil<T: class>(var Value: T); static; | |
| end; | |
| implementation | |
| uses | |
| System.SysConst, | |
| System.SysUtils; | |
| class function TObjectHelper.Cast<T>(const aValue: TObject): T; | |
| var | |
| lException: Exception; | |
| begin | |
| if Assigned(aValue) then | |
| begin | |
| if aValue is T then | |
| Result := T(aValue) | |
| else | |
| begin | |
| lException := EInvalidCast.CreateFmt('%s; actual type %s but expected %s.', | |
| [SInvalidCast, aValue.QualifiedClassName, T.QualifiedClassName]); | |
| raise lException; | |
| end; | |
| end | |
| else | |
| Result := nil; | |
| end; | |
| class procedure TObjectHelper.FreeAndNil<T>(var Value: T); | |
| begin | |
| System.SysUtils.FreeAndNil(Value); | |
| end; | |
| end. |
The Wiert Corner - irregular stream of stuff 
Thaddy said
I like it very much and proposed it for FreePascal, where we can simply write:
procedure FreeAndNil(var obj:T);
var
temp: T;
begin
temp := obj;
obj := nil;
temp.free;
end;
It indeed solves some problems associated with FreeAndNil.
(Actually I used to forbid its use..)
thaddy said
Something goes wrong here
procedure FreeAndNil(var obj:T);var
temp: T;
begin
temp := obj;
obj := nil;
temp.free;
end;<code.
thaddy said
Any quick way around the escaped arrows?
rvelthuis said
FWIW, in which way is your Cast better than a simple
as? ISTM it does the same and does not require a record helper. And why do you assign the exception to a variable lException before you raise it?jpluimers said
It is better in the sense that you get more context on when things go wrong.
That’s also why
lExceptionis there: if you put a breakpoint, you can immediately see the context spelled out for you before the exception even happens.In practice that is a real life saver, especially when you land a project maintaining sources that are in part not of your own quality standards.
Rudy Velthuis said
Waaaaaah! Do not make it “safer” to use FreeAndNil. Simpy advise people to avoid it as much as possible.Code that needs FreeAndNil is most of the time smelly. FreeAndNil is not a safety belt. It is something that looks like a safety belt.
jpluimers said
I think this improved version is indeed a safety belt. It at least saved me from quite a lot of accidents both in the form of wrongly freed interfaced references and ensuring a nil pointer gets the proper exception when dereferenced (instead of some hard to track down memory overwrite).
It still means you need to know what memory and pointer operations actually do, but it makes it a lot more manageable.
Of course your mileage might very.
One thing I am contemplating about is to call it
NilAndFreeas the implementation in the RTL System.SysUtils.pas is very similar to https://github.com/synopse/LVCL/blob/master/SysUtils.pas#L2970rvelthuis said
Anything that promotes the use of FreeAndNil is a problem. FreeAndNil is most of the time used for the wrong purpose. If FreeAndNil is needed to make code “safer”, usually something much more fundamentally is wrong. If FreeAndNil solves, or at least seems to solve an occurring problem, it is never investigated and never really solved. So instead of making it “safer” to use FreeAndNil, rather advise people to avoid it like the plague, even if it seems to solve something.
jpluimers said
For the same reason, you can argue that setting anything to nil would be void with the exception of breaking circular references.
The practice is that code has bugs. Most of them are not even fundamental issues, but small glitches. Regardless of the classification, unless you have unlimited time, you cannot prevent all of these bugs. You can hoever make sure that when they happen, you have enough pointers (pun intended) to find the cause as quickly as possible.
That’s what where nilling pointers and therefore a good version of FreeAndNil can be very helpful.
Uwe Raabe said
May be it is nit-picking, but
FreeAndNil(instance);
is not the same as
instance.Free; instance := nil;
There might be cases where the order of the call to Free and setting the instance to nil may be important. Especially when you have to admit that you may not be aware of the consequences in the code you are working on.
jpluimers said
Indeed, you are totally right. It is one of the reasons I wrote https://gist.github.com/jpluimers/31a036b7c91bc121c422fcb1a0adbd39 (which I added to another comment, but will add to the post shortly)
Adam Lipovský said
I am sorry, but I didn’t catch the point of this article. When I use FreeAndNil, I HAVE TO KNOW what I am doing – what object I am freeing. If I work with interfaces, I do not free them at all – they are reference counted and so they free on they own.
FreeAndNil() is only for cases, when I have repeatedly used reference which I have to test against nil – Assigned(). In case of locals, instance.Free is well enough.
jpluimers said
You hit the nail on the had with
Often when maintaining code, you do not always know the consequences of a refactoring from
TObjectreferences tointerfacereferences:FreeAndNilcaseThe underlying problem is that
FreeAndNilhas an untypedvarparameter.So you can call it with a reference to an interface, especially existing code that you have already refactored the reference from a
TObjectto aninterfacereference.If you do that, it regards it as a
TObject, then sets the reference tonilwithout realising it is in fact aninterfaceso theFRefCountis not decremented, then finally frees the instance.This causes all sorts of problems in two cases:
FRefCountis zero before freeing (still bad: even though usually you get a descriptive exception, but often with a hard to find stack trace)I just found back some code I threw together a while ago with a more typesafe
FreeAndNil: https://gist.github.com/jpluimers/31a036b7c91bc121c422fcb1a0adbd39rvelthuis said
If you know what you are doing, you dont need
FreeAndNil. I probably never needed/used it, in all the years it exists.