A case when FreeAndNil is your enemy

Posted by on in Blogs
It seems that my previous post about FreeAndNil sparked a little controversy. Some of you jumped right on board and flat agreed with my assertion. Others took a very defensive approach. Still others, kept an “arms-length” view. Actually, the whole discussion in the comments was very enjoyable to read. There were some very excellent cases on both sides. Whether or not you agreed with my assertion, it was very clear that an example of why I felt the need to make that post was in order.

I wanted to include an example in my first draft of the original post, but I felt that it would come across as too contrived. This time, instead of including some contrived hunk of code that only serves to cloud the issue at hand, I’m going to try a narrative approach and let the reader decide if this is something they need to consider. I may fall flat on my face with this, but I want to try and be as descriptive as I can without the code itself getting in the way. It’s an experiment. Since many of my readers are, presumably, Delphi or C++Builder developers and have some working knowledge of the VCL framework, I will try and present some of the problems and potential solutions in terms of the services that VCL provides.

To start off, the most common case I’ve seen where FreeAndNil can lead to strange behaviors or even memory leaks is when you have a component with a object reference field that is allocated “lazily.” What I mean is that you decide you don’t need burn the memory this object takes up all the time so you leave the field nil and don’t create the instance in the constructor. You rely on the fact that it is nil to know that you need to allocate it. This may seem like the perfect case where you should use FreeAndNil! That is in-fact the very problem. There are cases where you should FreeAndNil in this scenario. The scenario I’m about to describe is not such a case.

If you recall from the previous post, I was specifically referring to using FreeAndNil in the destructor. This is where a very careful dance has to happen. A common scenario in VCL code is to hold references to other component from a given component. Because you are holding a reference there is a built-in mechanism that allows you coordinate the interactions between the components by knowing when a given component is being destroyed. There is the Notification virtual method you can override to know if the component being destroyed is the one to which you have a reference. The general pattern here is to simply nil out your reference.

The problem comes in when you decide that you need to grab some more information out of the object while it is in the throes of destruction. This is where things get dangerous. Just the act of referencing the instance can have dire consequences. Where this can actually cause a memory leak was if the field, property, or method accessed caused the object to lazily allocate that instance I just talked about above. What if the code to destroy that instance was already run in the destructor by the time the Notification method was called? Now you’ve just allocated an instance which has no way to be freed. It’s a leak. It’s also a case where a nil field will never actually cause a crash because you were sooo careful to check for nil and allocate the field if needed. You’ve traded a crash for a memory leak. I’ll let you decide whether or not that is right for your case. My opinion is that leak or crash, it is simply not good design to access an instance that is in the process of being destroyed.

“Oh, I never do that!” That’s probably true, however what about the user’s of your component? Do they understand the internal workings of your component and know that accessing the instance while it is in the throes of destruction is bad? What if it “worked” in v1 of your component and v2 changed some of the internals? Do they even know that the the instance is being destroyed? Luckily, VCL has provided a solution to this by way of the ComponentState. Before the destructor is called that starts the whole destruction process, the virtual method, BeforeDestruction is called which sets the csDestroying flag. This can now be used as a cue for any given component instance whether or not it is being destroyed.

While my post indicting FreeAndNil as not being your friend may have come across as a blanket statement decrying its wanton use, I was clearly not articulating as well as I should that blindly using FreeAndNil without understanding the consequences of its effect on the system as a whole, is likely to bite you. My above example is but one case where you should be very careful about accessing an object in the process of destruction. My point was that using FreeAndNil can sometimes appear to solve the actual problem, when in fact if has merely traded it for another, more insidious, hard to find problem. A problem that doesn’t bite immediately.
About
Gold User, Rank: 84, Points: 11

Comments

  • Guest
    Mason Wheeler Tuesday, 16 February 2010

    Very nice explanation. But it's "in the throes of" with an E.

  • Guest
    Allen Bauer Tuesday, 16 February 2010

    Thanks, Mason. Fixed.

  • Guest
    Jim O'Brien Tuesday, 16 February 2010

    Effect not affect in the last paragraph.

    Nice post!

  • Guest
    Allen Bauer Tuesday, 16 February 2010

    Jim, I guess I know people actually read my ramblings :-). Fixed.

  • Guest
    Xepol Tuesday, 16 February 2010

    I've rewritten this post about 10 times now, pointing out the various flaws in what you have suggested here. Rather than a long, tedious(/annoyed) point by point here however, I think my response is best summed up with the following single statement:

    You have resorted to VERY flimsy rationalizations here.

  • Guest
    Marjan Venema Tuesday, 16 February 2010

    Nice post and I agree with your scenario. However, consider this: users of my class may not only reference my inner instances when I am in the throes of destruction. They may still have references to my inner instances around after I am done destroying myself and my inner instances. Something that easily happens whenever you expose an inner instance through a property. For example the Font or Canvas in many controls. Even when it is only readonly, the reference can get out. By not using FreeAndNil on my inner instances, I now am possibly faced with intermittent and hard to debug access violations as not all uses of the freed but not nilled instance will cause one - it will usually only happen when the memory formerly used by my (inner)instance is finally overwritten... By using FreeAndNil, while it may hide double free's and may cause memory leaks in the scenario you described, I at least ensure that anybody having dangling references around after I destroyed myself gets an AV as soon as they use it. For this reason, using FreeAndNil even in destructors actually has proven itself invaluable in exposing problems that would otherwise be very hard to debug. However, reading your previous post did cause an "aha.... d'oh" experience, as it addresses both problems. So, many thanks for that one!

  • Guest
    DK Tuesday, 16 February 2010

    "hard to find problem"?
    Any advanced memory manager will show you mem-leak on object that was allocated during destructor call. You'll solve the problem immediately.

    On other side: suppose you use Free. Now what? What if there will be no crash for some reason? How are you going to find this bug? What if (for amazing coincidence) code allocates buffer right on the same place as old object was? That is actually very likely - as MM tends to re-use memory (especially if buffer will be approx. the same size as object).
    You access the object - you damage your buffer. What if it was a buffer for saving object's user data to file? Oops, you just trashed your results and you have no idea why.
    Sure, you may use advanced memory manager with debug mode, but if misuse doesn't overflow any buffer - MM will not report anything.

    Still, a very good point, thanks for the post :D

  • Guest
    Primoz Tuesday, 16 February 2010

    While I understand your example (I think) I still think that the original post was misleading. In retrospect, it would be much better to write the current post first ;).

    The problem here is that various people jumped on the "FreeAndNil is bad" bandwagon without (I think) fully understanding why FreeAndNil is good for. And I still think that "Use FreeAndNil" should be a default for every Delphi programmer. Those, who doesn't understand why, would fare much better by using it at all times. The small minority of better informed people would sometimes NOT use it, but then they would know the reason why and would know how to defend the choice.

  • Guest
    Lachlan Gemmell Tuesday, 16 February 2010

    How about adding a FreeThenNil procedure that we obsessive compulsive nil'ers can use without risking side affects in destructors?

  • Guest
    Michael Skachkov Tuesday, 16 February 2010

    @Allen

    it seems to me that while trying to prove that using FreeAndNil everytime means 'bad design' you provide the example that is 'bad design' itself :)

    so, if you are even correct, then why to replace one bad design by another? :)

    if you don't think about lifetime of your objects of class you are about to write, then why to appeal to FreeAndNil calls?

    what i mean here is: first you need to think about class(or code) in general and decide how it will work and just then start implementing. i see no reason why you will not be able to use FreeAndNil in this case. instead you link outside code to your class's private fields (do you call it good design, huh?) and then explain that 'i should not use FreeAndNil here because it may result some problems then'.

    i'm sorry, but the design flaw here is in the way you're making your class, but not in place in destructor's code then.

    FreeAndNil gives certain benefits for debugging your code. yes, having strict AV may look a bit dirty way of debugging, but it is:
    a) simple to understand
    b) easy to locate

    if work in large projects, the simplicity of overall development process takes one of major requirements, otherwise there is a risk that project will never be completed. this IS important in real life.

    what you are talking about is a good explanation, as people mentioned above, however it contains potentially more problems and the worst thing is that they are most likely hard to debug/fix - especially in deployment terms.

    PS. of cause, i can think about i'm making mistake here or thinking wrong way. the best way to know that is actually - look at real example. if you have time, pls make simple project that we can compile on our computers. hypothetical thoughts do not look convincing. it's hard to understand why it may be necessary(or OK) to play tricks with pointers in destructor's code. well, i do think that if you need to do that, then that is already a signal about bad design.

  • Guest
    Will Watts Tuesday, 16 February 2010

    @Allen: So, let's get this straight: you are advocating leaving a pointer to dead object un-nil'd during destruction, so that you /might/ get a crash instead of definitely getting a memory leak?

    This seems pretty thin. Remember your subject is 'FreeAndNil is your enemy', not 'If you mess with a component with csDestroying set, you deserve whatever you get'.

    Or maybe you aren't suggesting this. As you yourself say, it is arguable whether FreeAndNil helps or hinders here... but then that doesn't feel like much support for all this 'enemy' talk.

    I think the chap who had the right idea was the blogger who advocated a variation of FreeAndNil - say FreeAndDoom - which set the object reference to a known invalid value (I think he chose the value 1 cast to a pointer). If applied in this case that you describe, the too-late lazy initialisation would be prevented by the non-nil value in the Assigned() test, and the desired A/V would explode at once, in the right place, right by the buggy code.

  • Guest
    Lars Fosdal Tuesday, 16 February 2010

    After having read the two posts and comments, I'd suggest that using FreeAndNil in general will allow you to detect and fix more flaws than if you were not using it. Lazy allocation could be a hard to spot problem, but as already pointed out - that would show up as a leak.

    An alternative to FreeAndNil in the destructor, could be to assign a const DestructorPtr = Pointer(Integer(Nil)+1); (or some other magic number) - and make a FreeAndDestruct routine for use in destructors. If you then use an assert in the lazy allocator to check that the lazy pointer is different from DestructorPtr, you would also be able to catch framework misuse or uninteded re-creation of lazily allocated objects.

  • Guest
    Alex Tuesday, 16 February 2010

    As a compoment writer, I consider FreeAndNil a very useful resource. Since Delphi does not provide a memory managed environment, que responsibility of taking care of memory is always part of our lives and in such a context, FreeAndNil participates makes things easier and safer. There are situations when you can take less care and situations when your design must be more complex to guarantee a decent behavior of you class and FreeAndNil alone will not solve the matter. However, by my experience, using FreeAndNill is always good practice. The thing is that in some (very rare) cases, FreeAndNil is just *part* of the good solution.

  • Guest
    An Pham Tuesday, 16 February 2010

    The present case is Very weak and Wrong reference to object on destroy hence wrong design/usuage at first place.

    1. When Notification is invoked with opRemove, never use property way to access the variable -> no memory leak. Must use straight reference to the variable or better yet, use the passed in AComponent parameter itself.

    2. Trashing memory is most/more dangerous than a memory leak.

    3. You can easily detect memory leak with current tool but you will be extremely difficult to detect/debug with trashing memory or pointer points to a wrong data

    Cheers

  • Guest
    Warren Tuesday, 16 February 2010

    I'd still like to see an example. Basically, I can only say that when using FreeAndNil the way I use it, I have never had any problem with it. Nor have any people who use my components ever had a problem with FreeAndNil.

    I tend to comment my object references by "ref-only", and "clean". If delphi syntax supported some kind of decorator, I would probably use it:

    FMyObject1:TMyClassA; @(ref); // don't free me from destructor!
    FMyObject2:TMyClassB; @(owner); // don't forget to free me in the destructor!
    FMyObject3:TMyClassC; @(lazy,owner); // I could be nil! but if I'm not nil, free me in destructor

    Anyways, if you could come up with even the most contrived example, I'd be tempted to concede you have more of a point than here. I almost could imagine that this could almost have happened once, maybe twice, in the history of delphi programming. Making it, well, a non-factor.

    Even a single anecdote (this actually happened to me! watch out!) carries more weight than a verifiably leaky abstraction.

    If you check component state, and are aware of your component and class set-up and tear-down mechanisms, including lazily created objects, this bug will never in a million years be on your radar.

    Now, if you guys could find a way to fix Error Insight, and that nasty habit that RAD Studio has of not updating identcache files, leading to bogus error insight spews, and it turned out that this had something to do with FreeAndNil, then I'd give you the point. :-)

    W

  • Guest
    Allen Bauer Wednesday, 17 February 2010

    @Michael,

    "it seems to me that while trying to prove that using FreeAndNil everytime means ‘bad design’ you provide the example that is ‘bad design’ itself"

    I'm not saying that at all. My point is that using it as a "fix" for what is possibly a bad design isn't really a "fix" is it?

    @Xepol,

    "You have resorted to VERY flimsy rationalizations here."

    I'm not trying to rationalize anything here. I am trying to merely point out that there are cases where FreeAndNil isn't the right approach. I'm trying to guard against the extremes (always use it vs. never use it) and suggest that a deeper understanding of the system or consideration of the design and architecture is far better than relying on the some "magical" property of FreeAndNil.

    Another point I can make here that may seem like heresy is that I sometimes get the feeling that the use of FreeAndNil by some is an unconscious desire to have true garbage collection when it does nothing of the sort ;-). (oh boy, let the sparks fly on that one ;-).

  • Guest
    Xepol Wednesday, 17 February 2010

    Allen, of course you are trying to rationalize here.

    You have created a situation that doesn't NEED FreeAndNil to have exactly the same problem.

    You have combined Defered construction of resources and a usage pattern that can ONLY cause problems and blaming the method an object is freed for the problem.

    Frankly, FreeAndNil is the only part of your scenario that ISN'T actually a contributing factor to the end result.

    Yes, you can do defered creation of resources without FreeAndNil. In the destructor, you could use If Assigned(X) THen X.Free; If someone is using the notification method to do more than nil their reference then you still get the same problem.

    That is, unless you were to check for csDestroying in the componentState before initializing the defered resource, and instead raised a "Boy you are dumb" exception instead.

    I honestly can't anyone trying to do what you are suggesting because getting a notification that an object is being destroyed puts it in a very questionable state.

    FreeAndNil is not the problem in the scenario you present - it is the bad choices that follow.

    You have created a scenario that has nothing to do with your core topic (FreeAndNil), and added in a usage pattern that SHOULD cause problems and concluded that FreeAndNil is the cause of the problem.

    If is anything other than a poor rationalization, start your weekend now - you need a rest.

  • Guest
    Primoz Wednesday, 17 February 2010

    @Xepol,

    You should think in a larger context. VCL is a service infrastructure. There are clients (components) that depend on that infrastructure. Even worse - they become part of that infrastructure.

    Because of the way the infrastructure is designed (yes, the design is debatable; no this is not the point of the topic, let's put it aside), the client depends on some infrastructure internals. The client can either handle problematic situation (aka component destruction) correctly, or not. In the former case, everything works independently of the way the infrastructure is implemented.

    In the latter case, though, the infrastructure can either allow the client to proceed even if it is doing wrong or it can crash. Obviously, the best way would be for the infrastructure to explicitly state "the client sucks" but it can't do that. (I definitely don't know enough about all the hoops and loops that the VCL must jump through to work seamlessly with the Win32 API to event start thinking about whether this third option can be implemented or not. Again - a design choice.)

    What Allen argues for is that the crash is preferable to the client not knowing about the problem. Actually, I agree with that, but only if we state that the design is fixed and cannot be changed. I'd much rather change the design (but let me repeat again - I'm not saying at all that the design *can* be fixed).

    So the bottom line is - if you have a complicated design, it is sometimes better not to nil dead objects. At least that's how I understand the topic. I may be totally wrong.

    While I don't know if design can be fixed or not, I do agree with people saying that in that case, invalid reference should be set to TObject(1), not to nil and not to the old (destroyed) object. That way the code would crash always, not just if FastMM happens to be watching over the memory allocation problems.

  • Guest
    m. Th. Wednesday, 17 February 2010

    Consider the case of a multi-threaded app in which a thread can have 0 (zero) or one instance (IOW a singleton).

    The thread does by itself an operation and then it dies. So, the best thing for this would be to set its FreeOnTerminate := True;

    However because the very last command in the Thread's destructor isn't the FreeAndNil and, as you say, setting the TThread instance to nil elsewhere will have nasty side effects, instead of a simple check for the (in)existence of the above thread (if Assigned(myThread) then...) we need to do other hacks / structures (remember that we are in parallel programming case) in order to see if the thread has really died.

    Are you going to fix this?

    PS: I don't say that any object when it is freed should have its handle set on nil - just in the case of threads where this thing is more difficult to handle and only in the case in which FreeOnTerminate is set to True - case in which the developer says explicitly 'do your job alone and die'.

  • Guest
    Allen Bauer Thursday, 18 February 2010

    @m. Th.

    If you ever use FreeOnTerminate, you should never have an external reference to the TThread instance, singleton or otherwise. It is entirely possible to have a "FreeOnTerminate" thread run to completion and be freed before you could even assign the instance to a variable after returning from the constructor.

  • Please login first in order for you to submit comments