A case against FreeAndNil

Posted by on in Blogs

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.

About
Gold User, Rank: 84, Points: 11

Comments

  • Guest
    David Heffernan Friday, 05 February 2010

    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.

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

  • Guest
    Allen Bauer Friday, 05 February 2010

    David,

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

  • Guest
    Allen Bauer Friday, 05 February 2010

    @An Pham,

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

  • Guest
    Allen Bauer Friday, 05 February 2010

    @Arvid,

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

  • Guest
    An Pham Friday, 05 February 2010

    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

    I hope that FreeAndNil should be an inline function

  • Guest
    Arvid Winkelsdorf Friday, 05 February 2010

    I am one of those not using FreeAndNil() each and everywhere.

    Imo 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

  • Guest
    Jorge Friday, 05 February 2010

    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.

  • Guest
    Rudy Velthuis (TeamB) Friday, 05 February 2010

    @Allen

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

  • Guest
    Henrick Friday, 05 February 2010

    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.

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

  • Guest
    Henrick Friday, 05 February 2010

    @Allen

    It'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.

  • Guest
    Primoz Friday, 05 February 2010

    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.

    There'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.]

  • Guest
    Giel Friday, 05 February 2010

    I agree with everything Primoz said.

    The fact that Delphi allows dangling object references is a mistake IMHO.

  • Guest
    Will Watts Friday, 05 February 2010

    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.

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

  • Guest
    Christian Wimmer Friday, 05 February 2010

    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.

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

  • Guest
    Eric Friday, 05 February 2010

    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.

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

  • Guest
    Xepol Saturday, 06 February 2010

    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.

    Honestly, 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.

  • Guest
    runner Saturday, 06 February 2010

    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.

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

  • Guest
    Pete R. Sunday, 07 February 2010

    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.

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

  • Guest
    David Heffernan Sunday, 07 February 2010

    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.

    As 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!

  • Guest
    Daniel Bernhard Sunday, 07 February 2010

    I still remember when I was new in delphi. I actually had much more problems about "Free" itself.

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

  • Please login first in order for you to submit comments