PSA: Don't use FreeAndNil

[Warfley]

I repeatedly see the usage of FreeAndNil for local variables in code, and often read in this forum the suggestion to use FreeAndNil because it is safer. I think that this is not just wrong, but makes it actually much harder to find bugs in the code. And I want to outline why, in an well engineered piece of code, you should not use FreeAndNil, unless in very specific situations that warrant it, and most defenetly, not use it just everywhere like it's a magic enchant that keeps memory related bugs away.

First a few basics, I will talk alout about bugs and errors, as well as exceptions and crashes. For many people, especially beginners, those that are the main recipients of the "always use FreeAndNil" advice, these terms may be somewhat interchangable. But here I am going to use them very specifically.
An Error is when your program is in an unintended state. For example when you want to open a file, you expect the file to opend after the open function, but if, e.g. due to the file not existing, it can't be open, your program is now in the state that you expect the file to be open, but it's not because it can't be opend. You have an error. But errors can be more subtle than that, e.g. if you have a Vector type with X and Y coordinates, and that vector should always be normalized (i.e. sqrt(X*X + Y*Y) = 1), and you somehow manage to write X and Y coordinates that do not satisfy this property, you have an error, even though you may not recognize it when it occurs.
A Bug is when you have code that is different from what actually intend it to be. Bugs come in all shapes, and can range from simple copy paste artifacts, where you forget to rename a variable name and use the copied name, other kind of bugs are when you use the same function, or you forget to check some conditions (e.g. divide by 0). A Bug does not necessarily cause an error, but can cause an error in certain situations.

For example:

Code: Pascal  [Select][+][-]

1. var
2.   sl1, sl2: TStringList;
3. begin
4.   sl1 := TStringList.Create;
5.   sl1.Free;
6.   sl2 := TStringList.Create;
7.   sl1.Free; // Bug: sl1 instead of sl2
8. end.  

This is clearly a bug (and as a result of Copy and Pasting a bug that happens at least to me in every larger project at least once), but when you run it, it (using a "normal" PC with the standard memory manager) acctually does not produce an error, because it just happens that the memory manager allocates sl2 in the same memory as sl1 formally resided in, so sl1.Free is actually the exact same as sl2.Free.

So to summarize, a Bug is the piece of code that is wrong, but not necessarily produce an error when executed. Your goal as programmer is to reduce the number of bugs in your code. So you need a way to measure these bugs when they occur. This can be done with plain simple code reviews or static analysis. Another tool we as programmers have is testing the code, when the bug does not produce an error in the tested scenario, there is no way to find the bug, because everything is working as intended.
So as counter intuitive as it may seem at first, we actually want to turn our bugs into errors. Because errors can be measured, found and traced back to the bug. So our goal is to write tests that triggers the bugs to cause an error and measure that error.

There are different ways a program can response to an error. If the error is easiely detected, it can crash the program, or it can be cought and some code can be executed that tries to "fix" the errorneous state. E.g. if you try to open a file that does not exist, you may ask the user for a correct filename and try again.
In Pascal these two cases are often done with exceptions. When you open a File with Assign, and there is a problem, an exception will be raised to notify you of that error. You can either handle that exception, and try to get your program back into a working state, or if you ignore the exception your application will crash.

If you do not handle the error, a crash is actually the best case scenario, because then you at least know something went wrong. The alternative, if you don't notice the error, you may run in an errorneous state. For example, if you turn of exceptions for the File handling, when you can't open a file, simply nothing happens. There will be a global variable set, but if you don't check that (which is your bug), all of the following write operations will do nothing, but the user will not notice anything. This is by far the worst case situation, imagine a user working with your program for an hour or so, with the goal to produce a file, and then after hours of work, they see that the file was not created, and all their work was pointless.

So to recap, we want all of our bugs to trigger errors, and on error, we want to be able to detect them, and the best way to do so, is by actually causing a crash. When we talk about memory management, we have a few tools to turn errors into crashes. The two main ones are HeapTrc and Valgrind. In this thread I will mostly talk about HeapTrc, because it's the FPC native solution (and I am currently not on Linux to make Valgrind examples), but many of the points also hold for Valgrind.
So what I am now going to argue in the remainder of this post is why FreeAndNil is minimizing the effectiveness of those tools, with examples using HeapTrc, on how without FreeAndNil you would be finding bugs that FreeAndNil will coverup and therefore make harder to find.

Note: One of the main features of HeapTrc that will be used here is the heaptrc.keepreleased property, which enables you to find use-after-free bugs, by instead of actually freeing the memory on Free, it will keep it around, but taint it so it is unusable (at least for usage of Classes). This feature may not be feasable to be enabled on a whole program. But it should be the first thing to enable in UnitTests, which are specific test scenarios that only test single features of the application at a time, and because they are so small, there it is a must have.

Let's start first with the best case, when FreeAndNil is working as intended:

Code: Pascal  [Select][+][-]

1. var
2.   sl: TStringList
3. begin
4.   sl := TStringList.Create;
5.   FreeAndNil(sl);
6.   sl.CaseSensitive := True; // Use after Free error, because it's called on nil it crashes
7. end.

This is the typical reasoning why you should use FreeAndNil, if you use the variable after nilling it, whenever the memory is accessed. Note that using "heaptrc.keepreleased := True;" will have a similar effect, as the object gets tainted, and valgrind would also flag every freed object and any use after free would be detected.
Note that unlike FreeAndNil, Heaptrc and Valgrind will also flag all references to that freed object:

Code: Pascal  [Select][+][-]

1. var
2.   sl1, sl2: TStringList
3. begin
4.   sl1 := TStringList.Create;
5.   sl2 := sl1;
6.   FreeAndNil(sl1);
7.   sl2.CaseSensitive := True; // sl2 is not nilled, even though it is the same freed object
8. end.

So even at it's best, FreeAndNil can help you spot some bugs, that you can also spot using the other tools at your disposal.

But where it gets really bad is when looking specifically at double free bugs, due to the interaction between Free and Nil (no pun intended). Free does not just Free an object, that is done by the destructor Destroy. Free checks if the object you call it on is nil, and if not it calls Destroy. Meaning if an object is set to Nil, Free (and therefore FreeAndNil) will simply do nothing.
Take this most simple example for a double Free:

Code: Pascal  [Select][+][-]

1. var
2.   sl: TStringList
3. begin
4.   heaptrc.keepreleased := True;
5.   sl := TStringList.Create;
6.   sl.Free;
7.   sl.Free; // Bug: Double Free
8. end.

In this setting it will always trigger a crashing error, but as we have seen in the example above, a double free may not always trigger a crash or even an error, if the memory that sl points to has been reallocated for another object in the meantime. This is why in this example we set "heaptrc.keepreleased := True;", so the memory is tainted and a double free will always trigger a crashing error. So this is good code, when we run it, it crashes, we spot the bug and correct it.
What happens if we use FreeAndNil? Well... nothing. Because FreeAndNil sets sl to Nil, so the second FreeAndNil will simply do nothing. This means, that here FreeAndNil will cause that Bug to not produce an error in the first place, which means we cannot detect this bug. While saving the programmer from an error being triggerd, also means that the programmer will likely not find that bug. So while it does not do harm to the program (in that it prevents an error), it also actively prevents the programmer from finding that bug and therefore improve their Code.
Note that the usage of FreeAndNil, by avoiding the call to the destructor, will also avoid the ability of HeapTrc or Valgrind to find this Bug, so if you use FreeAndNil you actually hinder the other tools to properly do their job.

It is similar with the very first example from this post:

Code: Pascal  [Select][+][-]

1. var
2.   sl1, sl2: TStringList;
3. begin
4.   sl1 := TStringList.Create;
5.   sl1.Free;
6.   sl2 := TStringList.Create;
7.   sl1.Free; // Bug: sl1 instead of sl2
8. end.  

Without Heaptrc, Valgrind or FreeAndNil, as previously shown, this is currently a bug that does not produce an error. With FreeAndNil, the sl1.Free will simply not do anything, which causes a memory leak, which actually is an error, and detectable, but only at the end of the execution and not that easiely tracable. HeapTrc and Valgrind will give you a segfault crash on the second Free directly, making it much easier to spot and fix. Again FreeAndNil hinders the full potential of the other tools.

So to summarize, FreeAndNil can at most help you capture a subset of the Bugs that you can capture with other tools, while actively preventing the other tools from finding more bugs. It is therefore nearly always preferential to use the existing tools without FreeAndNil.

Lastly I want to say, there are good uses for FreeAndNil. For example, you write a Chat client, with your Form, when you loose connection, the connection object may be Destroyed. This means when the user clicks the "Send" button during that time, it should not work. In this case a good solution is, to on disconnect, FreeAndNil the connection object, and in the Send button, check if the connection is nil before using. The important part here is that this is a very specific design, where the Freeing can take place at different points (in time or in code), and you need to signal if the object was already freed or not. In the very most cases this is not the cases, but every Create has exactly one Free, and any more Frees are a bug that need to be detected.
So yes, in this very specific case FreeAndNil is a great solution, but that is the exception not the norm.

PS: I just want to note that the problem with FreeAndNil is simply the call to Free. Generally speaking this is actually more a Free than a FreeAndNil problem (even though it's the combination of those two that triggers the problems). If instead of Free simply an exception would be triggered when the object is nil, it would be much safer. Even more, just calling Destroy instead of Free would solve these problems, so a DestroyAndNil (what everyone can easiely write themselves) would solve all of the problems above, without giving up the convinience of nilling in case neither HeapTrc nor Valgrind is used (e.g. in production code, or during User Tests with the whole Program and not just the UnitTests).

So I could have actually titled this "PSA: Don't use Free", but Free is so widely used that getting rid of this habit is probably impossible, while for FreeAndNil, it's not that engrained in our brains

[Fred vS]

Wow, thanks Warfley for this explanation complete, clear, with prove and examples.

DestroyAndNil

I vote for this! 
Fre;D

[BeniBela]

I just spend the entire weekend changing some of my classes to managed records

PSA: Don't use FreeAndNil, Don't use Free

[runewalsh]

I find FreeAndNil very useful for objects that have only one reference, which most objects are. I even have my FreeMem zero the pointer by default, and renamed non-zeroing FreeMem to FreeMemWeak for the same reasons, and did a generic counterpart to dispose than does if Assigned(x) then begin dispose(x); x := nil; end;.

In my case, it’s not even a bug-catching tool, it’s a matter of semantics. Imagine your object has three nested objects and Clear method that destroys them, but the outer object continues to live and can recreate them over time, checking each with Assigned(FNestedObject).

There are only 1.01 problems with FreeAndNil:

1) Its argument is untyped. Changing your object type to a non-class will crash in a disgusting way, at best. If your new type is a static record that has an unrelated class instance at offset 0, things will be even worse...

1.01) Accepting the argument by var forces it into memory, which might reduce performance. Usually however FreeAndNil is called either on fields that are already in memory, or on local variables of a procedure that has try and thus already forced all of them into memory.

[jamie]

Sorry, I've been in this for a long time. I'll still be using FreeAndNil

[Fred vS]

From sysutils.inc (maybe useful for the discussion):

Code: Pascal  [Select][+][-]

1.  procedure FreeAndNil(var obj);
2.       var
3.         temp: tobject;
4.       begin
5.         temp:=tobject(obj);
6.         pointer(obj):=nil;
7.         temp.free;
8.       end;    

Imho, following the code, it should be renamed into NilAndFree. 

And this one from objpas.inc:

Code: Pascal  [Select][+][-]

1. procedure TObject.Free;
2.         begin
3.            // the call via self avoids a warning
4.            if self<>nil then
5.              self.destroy;
6.         end;

So, if the object did not call destroy before using FreeAndNil, nothing is freed because self was set to nil before calling free... 

[eljo]

From sysutils.inc (maybe useful for the discussion):

Code: Pascal  [Select][+][-]

1.  procedure FreeAndNil(var obj);
2.       var
3.         temp: tobject;
4.       begin
5.         temp:=tobject(obj);
6.         pointer(obj):=nil;
7.         temp.free;
8.       end;    

Imho, following the code, it should be renamed into NilAndFree. 

And this one from objpas.inc:

Code: Pascal  [Select][+][-]

1. procedure TObject.Free;
2.         begin
3.            // the call via self avoids a warning
4.            if self<>nil then
5.              self.destroy;
6.         end;

So, if the object did not call destroy before using FreeAndNil, nothing is freed because self was set to nil before calling free... 

No self is not nil because temp that used to call free is not nil. As for the rename, delphi compatibility prevents that.

[Warfley]

In my case, it’s not even a bug-catching tool, it’s a matter of semantics. Imagine your object has three nested objects and Clear method that destroys them, but the outer object continues to live and can recreate them over time, checking each with Assigned(FNestedObject).

This is  pretty similar to the example I've outlined in my last paragraph, yes there are cases in which FreeAndNil is semantically useful, but those are rather the exception. You will encounter many one off objesct especially for local variables, such as FileStreams or Stringlists that are created once and then freed and never heard of again. And there is no reason to use FreeAndNil on them all the time, just because it is useful in another Situation. You won't unser an axe to drive in a skrew because it's good at chopping wood.

I think I sufficiently outlined with the examples in my first post how FreeAndNil reduces the effectiveness of other, more valuable, debugging tools. The problem is that it can often hide errors that you would otherwise be finding, because it avoids errors rather then helping you to find them

From sysutils.inc (maybe useful for the discussion):

Code: Pascal  [Select][+][-]

1.  procedure FreeAndNil(var obj);
2.       var
3.         temp: tobject;
4.       begin
5.         temp:=tobject(obj);
6.         pointer(obj):=nil;
7.         temp.free;
8.       end;    

Imho, following the code, it should be renamed into NilAndFree. 
[...]
So, if the object did not call destroy before using FreeAndNil, nothing is freed because self was set to nil before calling free... 

No because Free is called on Temp not on Obj. This is actually quite clever design, because if Free causes an exception, the nilling will already have taken place, so it is guaranteed to be nilled without requiring a try-finally (and the overhead it brings with it)

[runewalsh]

This is pretty similar to the example I've outlined in my last paragraph

Indeed. I didn’t get to that part, sorry :D

those are rather the exception. You will encounter many one off objesct especially for local variables

I use it constantly, to the point of having this (pic). But I also strongly avoid local variables of class types, instead using managed records that FreeAndNil contained objects in their finalizers.

[Warfley]

Of course Managed Records (or more generally any type whose lifetime management is automatically handled) is always a better solution. I should defenetly reconsider creating a a record datatype libarary with all the common types re-imagined as managed records. My last attempt was struck down through the double Free bug of record constructors, which I used so much that it basically amounted to nothing working at all

[marcov]

I normally use .free in such cases. Not so much because of the double free case, but simply because I understood what freeandnil was for (and the whole freeandnil cult could go hang, just as the ones that would indiscriminately berate you for not using try finally for every allocation)

I would have said that if you could potentially test a variable for NIL than you need freeandnil. Since .free tests for nil, a double free in included in that definition, but I think the next time I it comes up I will more explicitly paint the scenario.

The case for me where it is most useful is e.g. during shutdown. I generally free memory on shutdown of my long running applications because that eases looking for leaks.

However due to that the unit finalization order can change, sometimes that would cause double free's if you are not careful and freeandnil solves that.

[rnfpc]

It is a little confusing for non-experts like me. In the other thread ( https://forum.lazarus.freepascal.org/index.php/topic,63300.0.html ), many were emphasizing that try/finally/free should be used regularly. Many examples of problems with Free/FreeAndNil mentioned in this thread appear to be due to typing errors. Omitting a positive feature just because it may cause problem when used incorrectly is like saying "avoid knife in kitchen since it can cause injury if used incorrectly". Probably, advice to beginners can be different from advice to expert users.

[Warfley]

You should always free your allocated objects and Try-Finally helps immensly with it. This is just for using FreeAndNil, vs a normal Free (you must do one of those, the question is just which to choose). And you often see people that tell you to always use FreeAndNil (instead of .Free). Here I want to contest that idea that FreeAndNil is preferential to plain old Free, as, at least in testing (in production code this is a different story), it has no advantage over the tools you are probably already (and if not defenetly should be) using  like HeapTrc (especially in UnitTests with keepreleased) and Valgrind (requires Linux), while simultaniously diminishing the effectivness of those Tools.

And here the thing with FreeAndNil is that it hides bugs you could easiely find if you were just using plain old .Free.

But to repeat, no matter if you use Free or FreeAndNil (or Destroy), one of them should be always there. Always free your memory (it's not just because of memory consumption, the Constructor might execute important code that would be skipped if you don't Free).
And even when I (in the very last PS) say that actually using Free is the Problem, what I mean by that is that you can also call Destroy directly, there is no need to use .Free (which is used internally by FreeAndNil) at all, it is just a small function that checks for nil before calling Destroy which does the actual work.

This code:

Code: Pascal  [Select][+][-]

1.   sl := TStringList.Create;
2.   try
3.     // Do something with SL
4.   finally
5.     sl.Destroy;
6.   end;

is as good if not better than this code (unless in very specific situations):

Code: Pascal  [Select][+][-]

1.   sl := TStringList.Create;
2.   try
3.     // Do something with SL
4.   finally
5.     sl.Free;
6.   end;

Because Destroy does everything Free does, but also crashes if it is nil (when Free will just silently do nothing).
And the important part is that you want crashes. If something happens that you do not expect a crash is always better than ignoring it and doing nothing

[rnfpc]

Thanks for clarifying @Warfley.  So, we can say that as a general rule (especially for beginners), Destroy (in finally block) should be used instead of Free or FreeAndNil.

[Warfley]

Personally I think you shouldn't use Free. Because your code should be verbose. If you have that case that the variable can be nil, and this is part of your code design, you should simply add an if Assigned(VarName) then ... instead. So your code explicetly states that this special case can happen. If you always use Free, you are handling a situation that you may not expect at all, so basically you ignore an error.

My opinion is special cases should require special code to be self documenting. If you always do this special behavior, you are missing the errors where this should not happen, while not being able to see if you expect that or not in your code.

When you see the following in the code:

Code: Pascal  [Select][+][-]

1. if Assigned(myObject) then
2.   MyObject.Destroy;

You know directly: This code is designed in a way that MyObject may be nil here. If you see:

Code: Pascal  [Select][+][-]

1. MyObject.Free;

And Free is used everyhwere, you don't know if MyObject should sometimes be nil, or you would expect that this doesn't happen

But there is just a practical problem, I've first learned Pascal at the age of 8, the usage of "Free" is so deeply burnt into my brain, I can't simply stop using it. Old habits die hard. Also even if I would stop using it, everyone else is still using Free, and then using two different methods for the same thing is even more confusing to any outsider looking at that code.
So I think not using Free is just purely impractical, it's easier to just avoid nilling.

Another option may be somthing like this:

Code: Pascal  [Select][+][-]

1. procedure FreeAndInvald(var p: TObject);
2. begin
3.   p.Free;
4.   p := TObject(not 0);
5. end;

This sets the pointer to be all bits being 1, this is (at least on all modern x64 systems) an invalid address, so you would get a crash. But thats just a hack. Nil is specifically built for being a pointer with no valid memory behind it, so we basically just re-invent nil because we can't use nil