I really like the whole idea behind Stackoverflow. I regularly read and contribute where I can. However, I’ve seen a somewhat disturbing trend among a lot of the answers for Delphi related questions. Many questions ask (to the effect) “why does this destructor crash when I call it?” Invariably, someone would post an answer with the seemingly magical incantation of “You should use FreeAndNil to destroy all your embedded objects.” Then the one asking the question chooses that answer as the accepted one and posts a comment thanking them for their incredible insight.
The problem with that is that many seem to use FreeAndNil as some magic bullet that will slay that mysterious crash dragon. If using FreeAndNil() in the destructor seems to solve a crash or other memory corruption problems, then you should be digging deeper into the real cause. When I see this, the first question I ask is, why is the instance field being accessed after that instance was destroyed? That typically points to a design problem.
FreeAndNil itself isn’t the culprit here. There are plenty of cases where the use of FreeAndNil is appropriate. Mainly for those cases where one object uses internal objects, ephemerally. One common scenario is where you have a TWinControl component that wraps some external Windows control. Many times some control features can only be enabled/disabled by setting style bits during the creation of the handle. To change a feature like this, you have to destroy and recreate the handle. There may be some information that is stored down on the Windows control side which needs to be preserved. So you grab that information out of the handle prior to destroying and park that data in an object instance field. When the handle is then created again, the object can look at that field and if it is non-nil, it knows there was some cached or pre-loaded data available. This data is then read and pushed back out to the handle. Finally the instance can then be freed by FreeAndNil(). This way, when the destructor for the control runs you can simply use the normal “FCachedData.Free;” pattern since Free implies a nil check.
Of course there is no hard-and-fast rule that says you should not use FreeAndNil() in a destructor, but that little “fix” could be pointing out that some redesigning and refactoring may be in order.
Our codebase (500,000 lines) uses FreeAndNil always - we never call Free. The simple reason is that once you have freed an instance you shouldn't ever access its fields, as you so clearly state above.
ReplyDeleteIf you nil the ref then an access will always produce an AV. If you don't nil the ref then may get 'lucky' and get away with post-free accesses much the time depending on what happens with the MM. But you may hit an AV one time in a million and those are the hard faults to debug.
Once your program is correct then it doesn't matter whether you use Free or FreeAndNil since you aren't hitting the reference after the object has been freed. But it doesn't hurt to use FreeAndNil and it will flush out bugs - it has done so for us many many times.
David,
ReplyDeleteI would recommend that you use a debugging memory manager that doesn't recycle pointers and/or always invalidates the memory pages when freed. FastMM has several debugging modes of operation that can help you track down these kinds of things. Once you are satisfied that your application is working properly, merely go back to the regular memory manager. Another aspect of using FreeAndNil too much is that it also encourages you to pepper your code with unnecessary "Assigned() checks" which can also mask the very thing I was talking about in my post. This leads to the follow-on magic bullet of "always checking that the object is assigned before using it."
Yes, FreeAndNil can sometimes help uncover inappropriate references, but you can get the same effect by using other techniques that don't burden your regular non-debug code with the extra overhead of FreeAndNil() (not that it is very much).
@An Pham,
ReplyDeleteAccessing a non-nil pointer that crashes (using a Debug memory manager) makes it easy to distinguish between accessing an instance too early (prior to allocation) or too late (after freeing). Always returning freed instance references to nil adds an extra step in the debugging process.
@Arvid,
ReplyDelete"But as a matter of fact: Developers using FreeAndNil() to prevent crashes or exceptions..."
Agreed. That was the whole point I was trying to make.
"...should really learn about the cool Code Auditing features Delphi provides to check for design flaws."
Good advice. Which, BTW, we're putting in some effort to beef up this part of the product.
It is much easier to debug something that is nil then accessing some thing that is already freed. Remember that application is constantly evolved and code once is better
ReplyDeleteI hope that FreeAndNil should be an inline function
I am one of those not using FreeAndNil() each and everywhere.
ReplyDeleteImo the use of the destructor itself is the 1st choice if you want to free object instances in local scope or in owning destructors. Also when speaking of code readability and it's overall style.
But discussions about such things are always getting a bit philosophical like using Assigned() or nil.
Whether or not somebody uses FreeAndNil() - I cannot believe somebody really thinks of it as a Magic-Cleanup helper...
Garbage Collection takes place in developer's head - not through some magic code. Kind of old-school, I know.
But as a matter of fact: Developers using FreeAndNil() to prevent crashes or exceptions should really learn about the cool Code Auditing features Delphi provides to check for design flaws.
Afaik even the Professional SKUs are shipped with a basic set of Metrics & Audits, but they are seldom used from what I can tell...
Allen, probably get Chris Pattinson to blog about them and why QA is so important?
Cheers,
Arvid
I have always found the case for FreeAndNil() to be contrived. I think I saw one instance where it was merited and it was similar to the one you outlined above. In the case of the SO maxim to always use it, your initial reaction is correct -- it indicates some kind of design issue. It is a tool of very limited application and its overuse causes lazy programmers to stop thinking (or maybe lazy programmers have already stopped thinking). Arvid's comment about "Magic Cleanup helper" makes me smile. I would not miss FreeAndNil() if it were to disappear.
ReplyDelete@Allen
ReplyDeleteI have often said the same, in the Embarcadero forums.
Each time I see someone use FreeAndNil, it is either defended as "defensive programming" (defensive against design problems, I guess), or it is simply a sign of hiding wrong design. There are indeed cases where FreeAndNil is legitimate, but IMO, they are very, very rare. Using FreeAndNil and Assigned all the time is not defensive programming, IMO, it is often unnecessary programming.
I think there is another dimension to this. I don't primarily use FreeAndNil to *solve* problems (except for ephemeral objects as described by Allen), but to *cause* problems for testing purposes.
ReplyDeleteIf class TA has an object field FB that is created by TA.Create and destroyed using FreeAndNil in TA.Destroy, stray TA pointers might be detected with a non-negligible probability by checking Assigned(FB).
Of course, this should ideally only be done during debug. You shouldn't deliberately incorporate this trick into your design (because chances are it won't work, for a number of reasons), but that doesn't mean it is a bad idea to use such tricks consistently, to help isolating bugs elsewhere.
@Allen
ReplyDeleteIt's a good idea for application developers to use the debugging memory manager, but it's not an ideal option for e.g. library code. Using thing such as Assert(Assigned(FB)) might be a way to ensure that the code isn't used contrary to it's design contract.
FWIW, I believe the design by contract language features of Prism work essentially like this under the hood.
Of course, one should always use debugging memory manager, but one should also always use range checking, I/O checking, and FreeAndNil. I go in such lengths that also use DSiFreeMemAndNil (a FreeMem wrapper), DSiCloseHandleAndNull (for kernel objects) and so on.
ReplyDeleteThere's a simple reason why - because it's simpler to find a problem if you can see the point of failure in the debugger as opposed to finding it through the fastmm log. And there's another reason why - the code may not fail during the testing but it sure will fail on the client machine where you *don't* have debugging MM running. If the object is not nil, the error will appear in a different location and you may not be able to track it to the original cause.
Of course, there are situations where those approaches should not be used, but as a good programmer you're recognize them and you'll know why you're doing something different. And if you're not a good programmer than the mantra "always use FreeAndNil" will definitely help you.
[Side note: We had a bug in one of our servers which was triggered only on a client site - and on the production server. I was searching for it for more then a year (on and off, of course, mostly off as the crashes were very irregular, maybe once every two weeks). At the end I found a reason (purely by chance) - my code was destroying an object, passed destroyed instance to an event handlers, and only then nilling the pointer. Stuuuuuupid, but it cost us oh so much time to find it. FreeAndNil would prevent the problem. Sadly, all this happened before FastMM was invented.]
I agree with everything Primoz said.
ReplyDeleteThe fact that Delphi allows dangling object references is a mistake IMHO.
I believe I am one of those who has recommended the use FreeAndNil on StackOverflow - not as a 'seemingly magical incantation' as satirically represented by Mr B., but as general good practice. I see no reason not to use FnN on every occasion, and have seen no arguments here that sensibly contradict the policy.
ReplyDeleteFor example, AB's suggestion that one should use a debugging memory manager until 'you are satisfied that your application is working properly' seems to be a form of teasing. The point of using FnN is exactly that is highlights code problems that are found *after* you are satisfied.
The utility of FnN was impressed upon me by an experience with the famous and excellent Virtual Treeview component. Long ago, in D5 days, when it was supported by a mailing list and its API changed every week, I spent a day or so identifying a bug that occurred, yes, intermittently in (yes again) the destructor sequence of the component. Naturally it was caused by using some object that had already been freed. It was incredibly tiresome to track down, and in the end I caught it by sticking in FnN's everywhere there was a Something.Free(), which gave me an A/V at the relevant line. It was a very satisfying bug to squash, but, as others have observed in similar circumstances, it was a very poor use of programming time.
If the anti-FnN lobby can demonstrate a similar waste of resource engendered by its use, I would be most interested.
I prefer to use FreeAndNil in destructors because I had to learn the hard way. Once I didn't do it this way and randomly got an AV in the destructor when freeing an object. I couldn't find out until I used FreeAndNil there. WELL, maybe a beginner would think that it solved the problem. But I knew, that the instance pointer MUST have been correct because the inner object was never freed anywhere else. So why did it happen? In the end I found out that I accidently freed the object itself a second time but because the class code still existed in memory it worked. So I use FreeAndNil even in destructors because in this way and situation (again) I can check the properties, and if I find an incorrect state (e.g. here: all inner class instances are nil) then I know that the problem comes from the incorrect usage of the class itself.
ReplyDeleteI find it disturbing that people think that any function in a programming language can do magic (I had an epiphany.). Do not trust anything what a computer does! Human beings engineered these machines and wrote these programming lines. "Errare humanum est", I could say.
As a library programmer I always try to check parameters for invalid values before processing them. If I don't allow value 500 why should I allow 0 (or nil) if it isn't intended to be correct?
I don't allow nil value but I don't check for corrupted pointers. They need to crash, I say.
I'm a little bit anxious about ASSERT because I experienced that people tend to turn off the option when they come along such an exception instead of checking their code.
Well, auditing features are good. But DO NOT rely on them exclusively. I wouldn't be surprised about how different the results are from different auditing software manufactures.
And of course they won't find everything.
Instead use a combination of the different static (e.g. code review) and dynamic tests (e.g. unit tests) available.
Write down your own code style guide and stick to it. It helps more than just arguing about such stuff.
Better than FreeAndNil() is FreeAndInvalid(), where the pointer is not set to nil but to a magic "invalid" object, for instance TObject(1) is a good such invalid object.
ReplyDeleteThis prevents the Assigned() check mess, .Free not doing anything when called a second time (a second .Free will now AV), and you get the benefits of an AV on every usage of the FreeAndInvalid()'ed object.
FreeAndNil can then be reserved to objects for which a nil object is part of the design.
Personally, I prefer FreeAndNil to .Free not as a bullet to solve problems, but rather as one that make it marginally easier to catch rogue pointer usage. After it is found, you have to track down WHY the pointer usage is rogue - in many cases, making the pointer NIL so you don't use it any more IS the solution to the problem. When it isn't, you dig deeper to see what is the problem.
ReplyDeleteHonestly, I would love to see a compiler flag that added additional code that throws an exception when trying to access the fields, properties and methods of a nil reference. Yes, this makes the code much larger and slower - it would NOT be suitable for deployed code. That said, not every compiler flag IS suitable for deployed code.
Such a compiler flag would make it substantially easier to find the bad pointer WHEN it was used, as oppsed to when it finally ran wildly enough to cause a crash. (sadly, I had found there can be a significant different between the two!)
So, in summary FreeAndNil -> Highly valuable, but not a solution to everything, and a compiler flag that increases its value in DEBUGGING scenarios.
I agree with people who said that FreeAndNil is valuable. Especially Primoz. I have the same opinion. When the code is in production it is a LOT easier to find errors if there are no dangling pointers around.
ReplyDeleteI also had a hard time tracking such an error in production code once. And anything that lowers the chance of such incidents is welcome by me.
Whenever I used to free an object I also assigned nil to it to indicate that the object had been freed. Where did I get the idea for this? System.Assigned()! This function (supposedly) tells me that if the object is not nil then it has been . . . assigned (gasp)! It only made sense that I should assign nil to my object after freeing it so I don't make Assigned() into a liar.
ReplyDeleteI used to wonder why the system didn't go ahead and assign nil for me after freeing. Then I discovered FreeAndNil() and assumed Borland had the same idea that I did. Now I always use it. If my code is bad, I want to know that so I can fix it. I want it to fail every time. I don't want it to sometimes work and sometimes fail.
It doesn't look like Allen really understands why people use FreeAndNil! For what it's worth, as I stated above, our codebase uses it exclusively. We also don't have the swaths of "if Assigned" tests that are mentioned. Please Allen do elaborate on why using FreeAndNil leads to lots of "if Assigned" tests.
ReplyDeleteAs many people have pointed out FreeAndNil is used to flush out references after destruction. You state that debug memory managers like FastMM can find those. Well, they don't find them as well as FreeAndNil!
I still remember when I was new in delphi. I actually had much more problems about "Free" itself.
ReplyDelete1) Calling an instance method on a nil object IMO should be forbidden.
2) "self.Destroy" is IMO violating any memory guideline about good code (It feels like cutting off the branch you're sitting on) because:
2a) As in point 1, at the end of the method you're inside an instance method of a nonexisting instance
2b) You're actually freeing someone elses memory (e.g. the actual pointer to the object is owned by the caller. Not by the object itself)
3) Just because there are some places where you may have a 0..1 relationship and some people are too lazy to actually check whether you actually HAVE a reference doesn't make it a "good design". IMO the usual case is that you have an object that needs to be destroyed. In all those cases it would be an error if the object already IS nil.
So IF you want to complain about "Bad" code. Start checking about using "Free" as an easy way to release memory.
Now back to "FreeAndNil". IMO it's a critical mistake to keep garbage pointers. The "performance loss" of this one additional operation is really not an issue. However, the fact that you get an AV for sure when continuing to operate on this object is a very valuable fact.
@Primoz
ReplyDeleteWhere can we find the most up to date version of the wrapper pack containing DSiFreeMemAndNil etc (dsiwin32.pas)?
I did a Google search and found a number of versions from 2004-2007, but non seemed to be from the 'official' source. The most official link I can find is http://gp.17slon.com/gp/dsiwin32.htm and that link is dead.
@Daniel
ReplyDelete1) You can't just ban Free on a nil object without specifying how to handle all the knock on effects.
2a) At the end of Destroy the instance still exists. The memory is disposed of in FreeInstance which runs after the destructor completes. If you want to make comments like this then it really pays to read the code and know what you are talking about first. In fact you don't even need to read any code to know that what you say must be nonsense. If you really were executing code in a non-existent instance then what the heck could you do - you certainly couldn't do any of the stuff you do in a destructor.
2b) The object created the memory so it's natural for it to destroy the memory. I think of it that the memory is owned by the instance, but the object is owned by the caller.
@David,
ReplyDelete"It doesn’t look like Allen really understands why people use FreeAndNil!"
LOL! Oh, I completely understand all the various use cases, many of which are perfectly legitimate. I was merely stating that naive use as a "magic-bullet" without really understanding what is going on in your application is a recipe for further woes. To the StackOverflow reference, many times the question posted is from a relative novice in Delphi (or more specifically VCL/Components), so suggesting FreeAndNil() without properly warning them of the potential dangers, is not really "helping" them. It's like them asking how to repair a hole in their wall, and someone just suggests that they hang a new picture over it... Didn't *really* fix the problem, only masked it.
I will probably post a follow-on for other cases I've seen where FreeAndNil can actually *cause* a memory leak, when the application should have either crashed or the object is doing "normal" operations during the throws of destruction.
I specifically said, "Of course there is no hard-and-fast rule that says you should not use FreeAndNil() in a destructor"
So, *if* you are certain that using FreeAndNil() is going to uncover potentially latent bugs, then all the comments about its use also as a debugging tool are certainly valid. I, personally, would rather go a different route, but to each his own.
@Dave
ReplyDeleteYes you can find it in DSiWin32 I think.
The link "http://gp.17slon.com/gp/dsiwin32.htm" is valid. His site is just down at the moment. A day ago it was still running. I bet it will be back on shortly.
@Allen
ReplyDeleteI was merely stating that naive use as a "magic-bullet" without really understanding what is going on in your application is a recipe for further woes.
Not understanding what goes on in your app always ends in a disaster :). This is by no means isolated to FreeAndNil. Programing by coincidence comes to mind.
@Allen Bauer -> In production code, a memory leak can be preferable to instant failure. Many times, the leak can be within an acceptable margin.
ReplyDeleteIn particular, if the app is shutting down, leaking a few memory blocks is better than failing to shut down other more sensitive resources correctly. Memory gets cleaned up regardless once the app is gone. Sending shut down signals to external hardware on the otherhand - that requires that you actually allow that code to run instead of dying first.
@Allen
ReplyDeleteit would be very helpful if you demonstrated at least one real example of problem caused by using FreeAndNil.
developing applications with Delphi more than 13 years I actually never had any problem with that routine, but have lots of problems when didn't used it.
you know, using FreeAndNil is just good practice. I know that in many companies it is specified in guidelines as a requirement.
i would even say that if you are facing troubles when use FreeAndNil then THIS IS the design problem.
so far i see no arguments for not to using FreeAndNil. so i would appreciate very much to look at example
@Dave White: The link is correct, but the 17slon.com host was down for two days due to a faulty disk :(
ReplyDeleteEverything is working fine now.
I find that leaving the object or resource id not to nil or 0 state after freeing is almost causing countless hours of debug time and fixing bug is seem forever. Here is the case if setting it to nil or 0 state will give valuable error message to fix the problem or in Allen's word, bad design to be spotted
ReplyDeleteInstead of getting the actual error message, you get something completely from VCL codes. If Delphi does not come with source codes, you may never know how to fix the damp problem
You will get this message: "Thread error: handle is invalid (6)"
In this case, if the FHandle it set to 0 after CloseHandle, you will get the correct message and avoid codes in destructor to be skipped if values have been set to nil or 0 state
unit Unit1;
interface
uses
Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
Dialogs, StdCtrls;
type
TForm1 = class(TForm)
Button1: TButton;
Label1: TLabel;
Label2: TLabel;
procedure Button1Click(Sender: TObject);
private
{ Private declarations }
public
{ Public declarations }
end;
var
Form1: TForm1;
implementation
{$R *.dfm}
type
TTestThread = class(TThread)
protected
procedure Execute; override;
public
constructor Create;
destructor Destroy; override;
end;
const
CTestSleep = 2000;
var
ExecuteCounter: Integer;
ThreadCounter: Integer;
procedure TForm1.Button1Click(Sender: TObject);
begin
Label1.Caption := Format('Before %d - %d', [ThreadCounter, ExecuteCounter]);
try
TTestThread.Create;
Sleep(CTestSleep * 2);
finally
Label2.Caption := Format('After %d - %d', [ThreadCounter, ExecuteCounter]);
end;
end;
{ TTestThread }
constructor TTestThread.Create;
begin
Inc(ThreadCounter);
inherited Create(True);
FreeOnTerminate := True;
raise Exception.Create('Test thread constructor error based on customer data');
Resume;
end;
destructor TTestThread.Destroy;
begin
inherited Destroy;
Dec(ThreadCounter);
end;
procedure TTestThread.Execute;
begin
Inc(ExecuteCounter);
Sleep(CTestSleep);
end;
end.
Good points.
ReplyDeleteYou're right that you must think first, when you see that FreeAndNil "magically" solves the problem - it may be a design problem.
Unfortunately, using MM in full debug mode (as replacement for FreeAndNil) is not an option for deployment - and defensive programming comes in handy here.
But what are examples of possible "bad things"? I can only imagine a "double-free". So why don't use some DestroyAndNil? :D
Several comments request an explanation of how "bad things" might happen. I presume the answer requires the assumption that the developer is using FreeAndNil as bug fix *solution*, rather than as a bug hunting tool.
ReplyDeletePresume, for instance, that class tA has an object field tA.fB, that tA.fB is destroyed in tA.Destroy, and - the bug - that the code that creates the tA instance is destroying it twice in two different places. The first symptom, if using fB.Free in tA.Destroy, would be access violations, and using FreeAndNil(fB) would presumably make those AVs go away (most of the time). However, it wouldn't fix the cause of the bug and it might lead to even worse symptoms if the instance memory has been used for another instance before the first one is destroyed the second time.
Hence, it is important to treat FreeAndNil as a debug tool only in situations like this. If using FreeAndNil in a destructor helps prevent AVs you have a very strong indication that the instance is destroyed twice, so that's the problem you have to solve.