FreeAndNil() - The Great Delphi Developer Debate

[dummzeuch 1506]

2 hours ago, David Heffernan said:

That's a bug to use that reference, so this issue can safely be ignored with regards FAN. 

I agree that it is a bug, but exactly because of this the issue can not be ignored.

[dummzeuch 1506]

1 hour ago, Attila Kovacs said:

that would be wasting resources

So does the check for <>nil in .Free

[David Heffernan 2347]

1 minute ago, dummzeuch said:

So does the check for <>nil in .Free

No it doesn't. How else are you going to allow constructors to raise exceptions? 

[David Heffernan 2347]

4 minutes ago, dummzeuch said:

I agree that it is a bug, but exactly because of this the issue can not be ignored.

You fix the bug, which is nothing to do with FAN. That's my point. 

[Vincent Parrett 754]

I recently spent an entire week trying to fix a bug where in heavily threaded situations customers were seeing random av's. The av's and the stack traces were very random, which made it very difficult to pin down. Debugging threads in delphi is painful at best - any changes to timing can completely mask bugs so using breakpoints was not going to work (and the bug would occur in around 4 out of 40 identical threads).  After spending days adding even more (pointless as it turned out) locks all over the show I wasnt' much closer to figuring it out.

 

I suspected it could be a reference counting issue (since I use interfaces a lot), so started sprinkling some FreeAndNils around the code (in destructors) - suddenly those random av's turned into nil pointer exceptions that were much less random. That confirmed to me that the issue was with accessing objects already free'd. The problem turned out to be that I was using weak references where I really needed strong references - a 70hr week of frustration turned into a 4 line change 🤦‍♂️

 

I really don't understand all the angst and debate about FreeAndNil - use it where it makes sense, or don't - just like any other RTL function or language feature. 

[vfbb 285]

31 minutes ago, Vincent Parrett said:

I recently spent an entire week trying to fix a bug where in heavily threaded situations customers were seeing random av's. The av's and the stack traces were very random, which made it very difficult to pin down. Debugging threads in delphi is painful at best - any changes to timing can completely mask bugs so using breakpoints was not going to work (and the bug would occur in around 4 out of 40 identical threads).  After spending days adding even more (pointless as it turned out) locks all over the show I wasnt' much closer to figuring it out.

 

I suspected it could be a reference counting issue (since I use interfaces a lot), so started sprinkling some FreeAndNils around the code (in destructors) - suddenly those random av's turned into nil pointer exceptions that were much less random. That confirmed to me that the issue was with accessing objects already free'd. The problem turned out to be that I was using weak references where I really needed strong references - a 70hr week of frustration turned into a 4 line change 🤦‍♂️

 

That's why I made a topic warning that Weak is not thread safe (confirmed by @Dalija Prasnikar). So cast Weak to strong reference, and even if the strong reference is not nil, it can represent a destroyed object, generating a totally random AV, and this is not documented anywhere on the internet. More information: https://en.delphipraxis.net/topic/4020-weak-reference-is-dangerous/?tab=comments#comment-34379

[Vincent Parrett 754]

3 minutes ago, vfbb said:

That's why I made a topic warning that Weak is not thread safe

I saw that, but I'm not using the built in Weak stuff - this is in XE7 Win32 before weak was available - I'm using my own weak reference library - it's not as convenient (need to use a base class) but has worked well for me for the last 12 years or so. I still take a hard reference from the weak and check that for nil rather than relying on the .IsAlive property when using it in threads.

[Stefan Glienke 2009]

Using FastMM with the setting to mark freed memory with a certain pattern ($80808080 by default iirc) would have revealed that issue within a few minutes fwiw.

Edited June 28, 2022 by Stefan Glienke

[Vincent Parrett 754]

I did use that, and it got me closer, but not close enough. 

[Dalija Prasnikar 1399]

5 hours ago, Vincent Parrett said:

I saw that, but I'm not using the built in Weak stuff - this is in XE7 Win32 before weak was available - I'm using my own weak reference library - it's not as convenient (need to use a base class) but has worked well for me for the last 12 years or so. I still take a hard reference from the weak and check that for nil rather than relying on the .IsAlive property when using it in threads.

How much you pay me to prove your Weak library is not thread safe 

 

Main problem with FreeAndNil supporters is that it is treated as silver bullet and it is not. Like @Stefan Glienke said, there are better tools for the job. And in multithreading scenario all bets are off. Of course, if you suspect something is out of order then you can add some help like calling FreeAndNil for debugging purposes. Been there, done that. But sprinkling them all over the code, just in case? Sorry, but that is hard no. It just pollutes the code and gives you false sense of security, because it will nil just one reference and all others will turn into dangling pointers.

 

In other words, if FreeandNil can save me 70 hours of work in one case, it would still cost me way more work hours, when I have to read all code with FreeAndNil wondering whether there is a real purpose behind that call or it is there just for luck.

Edited June 28, 2022 by Dalija Prasnikar

[Dalija Prasnikar 1399]

6 hours ago, Vincent Parrett said:

I suspected it could be a reference counting issue (since I use interfaces a lot), so started sprinkling some FreeAndNils around the code (in destructors) - suddenly those random av's turned into nil pointer exceptions that were much less random. That confirmed to me that the issue was with accessing objects already free'd. The problem turned out to be that I was using weak references where I really needed strong references - a 70hr week of frustration turned into a 4 line change 🤦‍♂️

 

One question... you say you are using interfaces and then that calling FreeAndNil pointed you into right direction. Why are you calling Free (or FreeAndNil)  on reference counted instance? If reference counting is disabled then the concept of strong and weak references is not applicable.

[Vincent Parrett 754]

16 minutes ago, Dalija Prasnikar said:

How much you pay me to prove your Weak library is not thread safe 

Zero, I know it's not thread safe.

17 minutes ago, Dalija Prasnikar said:

In other words, if FreeandNil can save me 70 hours of work in one case

It wasn't just freeandnil that solved the issue (if only). Just another tool in the box, which I used then removed. 

 

I still don't get why people get upset about FreeAndNil - I have much bigger and more important issues with delphi to deal with (like actual bugs in the ide/compiler/rtl/vcl) rather than worry about something so minor.  

[Vincent Parrett 754]

3 minutes ago, Dalija Prasnikar said:

One question... you say you are using interfaces and then that calling FreeAndNil pointed you into right direction. Why are you calling Free (or FreeAndNil)  on reference counted instance? If reference counting is disabled then the concept of strong and weak references is not applicable.

The freeandnil call was on a TStringList, which was owned/instantiated by an interfaced object. 

[Fr0sT.Brutal 900]

I don't realize the problem at all. FaN pro's - it could help find bugs and is useful for lazy init. Con's - procedure use instead of a method? Duh. Insignificant for me. I see no reasons to not use FaN everywhere,

Edited June 28, 2022 by Fr0sT.Brutal

[Dalija Prasnikar 1399]

28 minutes ago, Vincent Parrett said:

Zero, I know it's not thread safe.

I was kidding. I don't even have to look at your code to know it is not thread-safe  

28 minutes ago, Vincent Parrett said:

It wasn't just freeandnil that solved the issue (if only). Just another tool in the box, which I used then removed. 

Like I said, using it for debugging purposes when something is off, and for some reason you cannot pinpoint the problem with other tools is fine.

 

28 minutes ago, Vincent Parrett said:

I still don't get why people get upset about FreeAndNil - I have much bigger and more important issues with delphi to deal with (like actual bugs in the ide/compiler/rtl/vcl) rather than worry about something so minor.  

Problem is not in using FreeAndNil. It has its use case (beyond debugging purposes). The problem is using it everywhere, and then it obscures the intent of the code. When you FreeAndNil, your code is sending the different message that when you just Free. If you FreeAndNil that means such reference can be nil at any point and all code using such reference also needs to deal with scenario where reference is nil, either constructing new instance if the reference is nil, or skipping operations on nil reference. And that is drastically different code than one dealing with reference that cannot be nil.

[David Heffernan 2347]

1 hour ago, Dalija Prasnikar said:

Main problem with FreeAndNil supporters is that it is treated as silver bullet and it is not.

As you say, that's a problem with the developer rather than the function itself.

 

I personally use FAN everywhere and my debug builds use FastMM with all bells and whistles.

 

There aren't downsides to using FAN and it does make it easier to catch some defects. 

[Attila Kovacs 629]

8 minutes ago, Dalija Prasnikar said:

Problem is not in using FreeAndNil. It has its use case (beyond debugging purposes). The problem is using it everywhere, and then it obscures the intent of the code. When you FreeAndNil, your code is sending the different message that when you just Free. If you FreeAndNil that means such reference can be nil at any point and all code using such reference also needs to deal with scenario where reference is nil, either constructing new instance if the reference is nil, or skipping operations on nil reference. And that is drastically different code than one dealing with reference that cannot be nil.

 

poor freeandnil(), it has nothing to do with it, it's a low-level design pattern you don't like

 

 

[dummzeuch 1506]

Hm, wasn't the point of the first post in this thread that FreeAndNil isn't big enough of a subject to make a whole video about?

Two pages of discussion say otherwise.

[David Heffernan 2347]

24 minutes ago, Dalija Prasnikar said:

If you FreeAndNil that means such reference can be nil at any point and all code using such reference also needs to deal with scenario where reference is nil

This makes no sense at all. If you instead use Free and retain a stale reference to a destroyed object, then other code simply cannot know that the reference is stale. 

 

There are two main scenarios. 

 

Object destroyed by owner, either from another instance's destructor, or in a method with try finally lifetime. FAN is just a debugging guard against use after free. 

 

Or a reference to an instance whose lifetime is dynamic. Perhaps created and destroyed multiple times by its owner. Then nil is used to indicate the state that the object is currently not in existence. 

 

It seems bizarre to me that anybody could advocate carrying around stale pointers. 

[Dalija Prasnikar 1399]

35 minutes ago, David Heffernan said:

This makes no sense at all. If you instead use Free and retain a stale reference to a destroyed object, then other code simply cannot know that the reference is stale. 

I am talking about scenarios where object instances are lazily constructed or can be nil at some point for other reasons.

 

For instance, think of list of children that is nil if there are no children and is only valid when child is added, but is also destroyed when last child is removed. In such case you would check if the instance is nil and construct the list before adding an new child. If you remove some child and there are no children left in the list, you would then call FreeAndNil on such list. If you are doing some processing on the children, you would just check if the list is nil and skip processing code in such case, because there are no children that need to be processed. So being nil is valid value for such reference and it can become nil at various stages during its owning instance lifetime. 

 

This is drastically different coding pattern than having some list which will be constructed in constructor and destroyed in destructor and will be valid reference in the rest of the code. For such references I would use Free and not FreeAndNil.

 

This is the distinction of the code intent I am talking about. If you use FreeAndNil everywhere then you lose the intended design and usage. Of course, tehre are other ways to detect how the instance should be used, but nothing is a problem in simple code and scenarios. In more complex code losing the intent is much harder to deduct and it can mean the whole world of difference. I would say that you can solve that problem with leaving comments in the code, but I have yet to see such comments in code that indiscriminately use FreeAndNil. 

 

If I want to detect stale reference access I would use other tools, instead of relying on FreeAndNil.  

 

35 minutes ago, David Heffernan said:

Object destroyed by owner, either from another instance's destructor, or in a method with try finally lifetime. FAN is just a debugging guard against use after free. 

 

Or a reference to an instance whose lifetime is dynamic. Perhaps created and destroyed multiple times by its owner. Then nil is used to indicate the state that the object is currently not in existence. 

Yes, those are the two scenarios. And for the first one there are better tools, especially since FreeAndNil only nils one reference and you may have other stale ones. Yes, I have occasionally used FreeAndNil for detecting stale pointer access, but only as temporary debugging tool and because I was too lazy to setup FastMM for some reason (read not important project). In other words, I would replace it with Free when the code is fixed (I don't think I have ever used it for debugging purposes more than few times)

 

35 minutes ago, David Heffernan said:

It seems bizarre to me that anybody could advocate carrying around stale pointers. 

I am not advocating that anywhere. That is why I am using Free (except for the second scenario with dynamically constructed objects), because I am not touching stale pointers, so using FreeAndNil is pointless. 

[Sherlock 663]

So, to wrap it up: Be a genius and have your decades old application rely on an object being available at all times. Or just double check to make sure. The penalty for that is being frowned upon by the real geniuses...or not.

[Lajos Juhász 293]

17 minutes ago, Sherlock said:

So, to wrap it up: Be a genius and have your decades old application rely on an object being available at all times.

No, you missed the part that you have only to learn how to manage objects. Using FreeAndNil you solve nothing. If you're unsure how to write quality code I suggest use always interfaces.

[David Heffernan 2347]

2 hours ago, Lajos Juhász said:

If you're unsure how to write quality code I suggest use always interfaces.

Wrong solution. Right solution is to learn what you don't know how to do. No shortcut. Certainly not by just saying "use interfaces". 

[David Heffernan 2347]

3 hours ago, Dalija Prasnikar said:

This is the distinction of the code intent I am talking about. If you use FreeAndNil everywhere then you lose the intended design and usage. Of course, tehre are other ways to detect how the instance should be used, but nothing is a problem in simple code and scenarios. In more complex code losing the intent is much harder to deduct and it can mean the whole world of difference. I would say that you can solve that problem with leaving comments in the code, but I have yet to see such comments in code that indiscriminately use FreeAndNil. 

My code uses FAN rather than free as a rule, and I don't recognise the problem you describe. That's my practical experience. 

[Lajos Juhász 293]

21 minutes ago, David Heffernan said:

Wrong solution. Right solution is to learn what you don't know how to do. No shortcut. Certainly not by just saying "use interfaces". 

My first suggestion was and still is to learn how to write a correct code. Interface is much better than constantly using FreeAndNil as band-aid solution.

 

For example free and nil will not save you here:

 

var

  x1, x2: TMyClass;

 

begin

  x1:=TMyclass.create;

  .....

  x2:=x1;

  ......

  FreeAndNil(x1);

 

  x2.SaveTheWorldAndMakeItABetterPlace;

end;

Here maybe you will think you have an excellent code as there is FreeAndNil, however that will not help and x2 is still a stale reference.

 

Edited June 28, 2022 by Lajos Juhász