Recent

Author Topic: PSA: Don't use FreeAndNil  (Read 16814 times)

Warfley

  • Hero Member
  • *****
  • Posts: 1865
Re: PSA: Don't use FreeAndNil
« Reply #30 on: May 20, 2023, 12:20:02 am »
And heaptrace and valgrind as far as I understand doesn't help if that buggy program part isn't executed, e.g. if it's a corner case of the code that is only reached in very specific circumstances.

Yes, this is the main problem with testing code, is to figure out all the cases where things can go wrong. The most common way to do this is to simply create a lot of unit tests and use a code coverage metric to measure how well you test your code. That said, 100% code coverage, may not necessarily mean every path is explored.
Take:
Code: Pascal  [Select][+][-]
  1. if A < 10 then
  2.   MyObj.Free
  3. else
  4.   MyObj.DoSomethingElse;
  5. if B < 10 then
  6.   MyObj.Free
  7. else
  8.   MyObj.DoAThirdThing;
With two test cases A = 5 and B = 15 and A = 15 and B = 5, all instructions in this example will be executed once. But the double Free bug is not detected, because to do so you would need to cover all paths through tre program.

There are some ways to test all paths, for example in Uni I have worked on Symbolic Execution, which is a technique that emulates a program and basically at every branch forks the execution to explore each branch individually. This way a 100% path coverage can be reached, if the number of paths is finite (which is not always the case e.g. if you have an infinite loop that can branch every iteration you trivially have an infinite number of paths).

One such symbolic execution engine (the one I personally have worked with) is KLEE, which runs on LLVM. As FPC has an LLVM backend, trying to get fpc generated code to run with KLEE is actually on my long list of projects I'll never have time to do (it's not that easy in practice, as KLEE provides a runtime for some C APIs, all the syscalls that fpc generates directly, need to first be mapped onto libc calls before it can be used with KLEE, which will catch and emulate the C calls).
But in theory you can just use a software like KLEE to explore all possible paths, and generate yourself a bunch of unitttests from those results. Also while exploring KLEE already tests for memory errors, so use-after-free would be discovered by KLEE directly, and we have found a few actual bugs in the GNU core utils with it, so it's actually practically useful on real software.

But that was just a short digression into an unrelated topic

kupferstecher

  • Hero Member
  • *****
  • Posts: 603
Re: PSA: Don't use FreeAndNil
« Reply #31 on: May 20, 2023, 12:05:09 pm »
Perhaps it would be desireable to have a compiler option to change the behaviour of Free() between Free and FreeAndNil so that both versions could be used while testing the programm without rewriting the code.

Does anyone want to comment on this?

alpine

  • Hero Member
  • *****
  • Posts: 1356
Re: PSA: Don't use FreeAndNil
« Reply #32 on: May 20, 2023, 12:27:25 pm »
Perhaps it would be desireable to have a compiler option to change the behaviour of Free() between Free and FreeAndNil so that both versions could be used while testing the programm without rewriting the code.

Does anyone want to comment on this?

It can be easily done with a conditional directives. No need for compiler option.
"I'm sorry Dave, I'm afraid I can't do that."
—HAL 9000

kupferstecher

  • Hero Member
  • *****
  • Posts: 603
Re: PSA: Don't use FreeAndNil
« Reply #33 on: May 20, 2023, 04:20:20 pm »
It can be easily done with a conditional directives. No need for compiler option.

That would clutter the code, imho.

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #34 on: May 20, 2023, 05:22:33 pm »
You've got
Code: Pascal  [Select][+][-]
  1. SomeObject.Free;
  2. SomeObject := nil;

vs

Code: Pascal  [Select][+][-]
  1. FreeAndNil(SomeObject);

as someone pointed out FreeAndNil assigns the referenced object to a temporary variable then sets the original reference to nil and finally frees the memory via the temporary variable. So what have we achieved??? Now we can be assured that our original variable is "nil" even if our destructor blows up??? What benefit is this to anybody??? If your destructor is going to crash let it crash!!! Fix the damn destructor, don't obfuscate the problem with stupid hacks like FreeAndNil.

I may be old and sufferring from cognitive decline but I remember a time when there was no FreeAndNil. I've asked earlier for some examples where it is necessary to set a variable to nil. I've got no responses to that query! What do you guys think FreeAndNil does??? Today the implementation has been tightened up to accept only objects - who the hell would pass anything but an object to FreeAndNil in the first place??? Do I sound angry?

Which leads me to another point: "Free" is a member of TObject. You call "SomeObject.Free" - this is the natural way to do things in Object Pascal. There is no ambiguity. "FreeAndNil" is just some clumsy "functional/procedural" call where people were once able to pass anything they'd like and apparently they did! That's why the implementation had to be changed!

Again, can anybody offer a reason why it is necessary to set a variable to nil besides the example I offered earlier? Anything??? Anybody???

The OP writes Re: PSA: Don't use FreeAndNil and I agree.

Handoko

  • Hero Member
  • *****
  • Posts: 5389
  • My goal: build my own game engine using Lazarus
Re: PSA: Don't use FreeAndNil
« Reply #35 on: May 20, 2023, 05:58:55 pm »
Use or not use FreeAndNil is an interesting topic.

I am not a fan of using FreeAndNil but I can be sure to tell you all, that there are some very rare (or extremely rare) cases, you really have to use FreeAndNil.

As a hobbyist programmer, I should not argue with full time professional programmers. I prefer to let proofs to speak itself, I made a challenge here:
https://forum.lazarus.freepascal.org/index.php/topic,58420.msg435373.html#msg435373

I still haven't got any good demos showing the real need of using FreeAndNil.

You can ignore the link above, here I re-challenge anyone who believes we should always use FreeAndNil:
Show/write me a demo, I mean the full source code that I can download, compile, run and examine that using FreeAndNil, which if we change it to simply free only, the code will fail to run correctly.

And please do not provide the demo that showing newbie's bad programming practice, as I posted here:
https://forum.lazarus.freepascal.org/index.php/topic,58420.msg435401.html#msg435401

Using FreeAndNil or not is a personal preference. It is okay if you like to use it. But if someone says we should always use FreeAndNil, that is wrong. I have been using Pascal since TP 3, I only encounter one case that I have to use it. All the other codes of mine seem to run correctly without it.

I will show my code, which really requires the use of FreeAndNil but only after someone posted some demos that show the 'real' need of using FreeAndNil.

If no one can provide a good demo, I will keep believing that 99% of cases, Free only is good enough.
« Last Edit: May 20, 2023, 06:15:07 pm by Handoko »

Warfley

  • Hero Member
  • *****
  • Posts: 1865
Re: PSA: Don't use FreeAndNil
« Reply #36 on: May 20, 2023, 06:11:26 pm »
If your destructor is going to crash let it crash!!! Fix the damn destructor, don't obfuscate the problem with stupid hacks like FreeAndNil.
Exceptions are not necessarily errors, they are just an additional control flow way for handing data which was not expected or does not fit in the original return space.
Exceptions in Constructors and Destructors have a very special function, because this is the only way to truely interupt their execution. An exception in the constructor will stop the construction of the object and free the memory of said object, meaning you can basically undo the whole memory allocation the constructor does (what in contrast is not possible with a simple Exit).

In the destructor it's the other way around, an exception can be used to stop the freeing of the object.
An example for this may be, consider a Thread of some long lasting task. So you want to free it, e.g. the owner is being freed. Yet the thread is not finished, and the terminate may take some time, and you can't free the Thread until it is finished.
What you can do is in the destructor simply add a check if the thread is still running, if so raise an exception. The caller of the destructor can then catch the exception, and handle it otherwise (e.g. by setting FreeOnTerminate and then throwing the thread away to finish and free itself).

So Exceptions in the Destructor may be useful and are not just an error leading to a crash. And in such cases, having the behavior of FreeAndNil well defined is actually quite smart

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #37 on: May 20, 2023, 07:15:49 pm »
Quote
Exceptions are not necessarily errors, they are just an additional control flow way for handing data which was not expected or does not fit in the original return space.

I'm afraid I do not agree with you. This is a radically different subject and perhaps we may debate it elsewhere but for now I will simply posit that data that was not expected and does not fit the original return space are certainly "errors" that we can safeguard against. Exceptions are used to circumvent things that are beyond our control (i.e. lost socket connections, no more disc space...)

I'm hoping somebody can explain why "FreeAndNil(o);" is preferable to "o.Free; o := nil;".

alpine

  • Hero Member
  • *****
  • Posts: 1356
Re: PSA: Don't use FreeAndNil
« Reply #38 on: May 20, 2023, 07:25:57 pm »
@Warfley
Your sample (corrected here):
Code: Pascal  [Select][+][-]
  1.     if A < 10 then
  2.       MyObj.Free
  3.     else
  4.     begin
  5.       MyObj.DoSomethingElse;
  6.       if B < 10 then
  7.         MyObj.Free
  8.       else
  9.         MyObj.DoAThirdThing;
  10.     end;
Contains serious logic error, not just a "double-free" one.

In general, I can see the FreeAndNil is recommended in cases the variable scope is broader than it should be, and in that case, IMHO, the real problem is that the ownership is not clearly established, i.e. who will be responsible for freeing.

I believe that someone introduced it for their own convenience instead of:
You've got
Code: Pascal  [Select][+][-]
  1. SomeObject.Free;
  2. SomeObject := nil;

and then someone else announced it as a panacea which it is not.

*snip*
I will show my code, which really requires the use of FreeAndNil but only after someone posted some demos that show the 'real' need of using FreeAndNil.

If no one can provide a good demo, I will keep believing that 99% of cases, Free only is good enough.
+1
While I can't give you such an example, I'm quite curious what is the case when FreeAndNil is unavoidable. But I'm afraid I'll never know ;)
"I'm sorry Dave, I'm afraid I can't do that."
—HAL 9000

Warfley

  • Hero Member
  • *****
  • Posts: 1865
Re: PSA: Don't use FreeAndNil
« Reply #39 on: May 20, 2023, 07:43:00 pm »
I'm afraid I do not agree with you. This is a radically different subject and perhaps we may debate it elsewhere but for now I will simply posit that data that was not expected and does not fit the original return space are certainly "errors" that we can safeguard against. Exceptions are used to circumvent things that are beyond our control (i.e. lost socket connections, no more disc space...)

It's not an error because it does not lead to misbehavior. For example, if you have a socket and make it non blocking. If you read and there is no data, it will be an exception, in the sense that in the classical Berkley Sockets C API it returns -1 and sets errno, and a more high level Pascal API may use exceptions for it. But you made that socket explicetly non blocking, with the knowledge that reading when no data available is an option and will occur. If you made the socket non blocking this behavior is actually the behavior you want to have, and therefore it can't be an error, in the same way if I go to McDonalds and buy a cheeseburger, not getting a healthy meal is not an error but what I wanted.

Exceptions are just a way to extend the output space through additional control paths that carry information outside the normal return type. It's not that often used in Pascal, I think because Exceptions in Lazarus are anyoing, while in other languages exceptions that will be cought are usually not thrown in the debugger, but for example in Java it's very common, if you want to check if a file has a certain encoding, trying to decode it and if there was an exception you know that the file has not the desired encoding.

One example where I have used Exceptions quite a lot was in my STAX library. Basically the current design of Threads has the problem that you need to regularly check if you are terminated. Long lasting blocking functions (such as a read on a socket) basically mean your thread cannot be terminated until that function returned. So when I created STAX, I thought on how I can turn this polling mechanism into a notification mechanism, and what I did is when you are blocking you call the scheduler to be re scheduled. When you terminate that task, the scheduler will wake up the Task and raise an exception. This will prematurely interrupt the blocking operation, and because it was interrupted, it does not have an result, so instead the exception must be handled.
This allows any task to be stopped at any time, it gives the Task the oportunity to die handle the termination and do some additional stuff before stopping by handling the exception, but even if the exception is not handled, it will execute all the finally blocks before it will forcefully stop the task, making even the "non graceful" termination still completely safe.
And before anyone is inclined to call premature stopping of a thread an error, for example this termination mechanism is what I have used for a socket receive with a timeout, simply do a blocking call, and if the call has not returned after the timeout, the terminate exception stops the blocking call, and gracefully shuts down the task. So it's not an error, it is just a feature I use, as intendet to accomplish a certain thing

I've actually built something similar prototypically for Linux pthreads using pthread signals, which allow to seize the execution context of a thread and then can raise an exception in that context, it's really hacky and defenetly not in any way production ready, but this addresses a clear need that anyone who has ever worked with threads has probably encountered at one point.

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #40 on: May 20, 2023, 08:00:23 pm »
Quote
...extend the output space through additional control paths ...

Now we have entered the realm of science fiction. If it takes you more than a few sentences to make a point you have no point at all! You're just fooling yourself. Let's go back to Object Pascal 101:
  • Absolutely NO exceptions in the constructor
  • Absolutely NO exceptions in the destructor
Can we agree? Thank you.

Handoko

  • Hero Member
  • *****
  • Posts: 5389
  • My goal: build my own game engine using Lazarus
Re: PSA: Don't use FreeAndNil
« Reply #41 on: May 20, 2023, 08:01:33 pm »
*snip*
I will show my code, which really requires the use of FreeAndNil but only after someone posted some demos that show the 'real' need of using FreeAndNil.

If no one can provide a good demo, I will keep believing that 99% of cases, Free only is good enough.
+1
While I can't give you such an example, I'm quite curious what is the case when FreeAndNil is unavoidable. But I'm afraid I'll never know ;)

I can tell you a bit. I wrote that short program for Pascal Game Contest some years ago. Unfortunately, I couldn't finished it before the deadline. That was a side scrolling 2D spaceship shooting game, written using Lazarus purely using only TCanvas, no third party graphics library.

Due to the way I code, I needed to generate a list to store all the items (rocks, bullets, ships, etc) generated on runtime and used a public variable for optimizing the performance. That variable was a pointer point to a runtime generated object. The generation of that object, reading its value and freeing it were not in the some code block, it was distributed in multiple locations, and it needed to be freed/renewed repeatedly. And that was the problem. For someone who really understands FreeAndNil command, now should understand the reason when I said ... distributed in multiple locations and repeatedly renewing.

I thought I might able to 'fix' the code, for not using FreeAndNil. But refactoring it would be too hard for me. Maybe that was a flaw in the software design. Instead of 'fixing' the code, I simply chose to use FreeAndNil, problem solved.
« Last Edit: May 20, 2023, 08:16:43 pm by Handoko »

kupferstecher

  • Hero Member
  • *****
  • Posts: 603
Re: PSA: Don't use FreeAndNil
« Reply #42 on: May 20, 2023, 08:03:18 pm »
You can ignore the link above, here I re-challenge anyone who believes we should always use FreeAndNil:
Show/write me a demo [...], which if we change it to simply free only, the code will fail to run correctly.
[...]
Using FreeAndNil or not is a personal preference. It is okay if you like to use it. But if someone says we should always use FreeAndNil, that is wrong. I have been using Pascal since TP 3, I only encounter one case that I have to use it. All the other codes of mine seem to run correctly without it.

I think you misunderstood the reason of the suggestion to use FreeAndNil. If your code requires it, then its not a question of suggestion, but it just requires it (you may explicitly nil the variable instead). In my understanding FreeAndNil is suggested so that use-after-free situations are detected and these are quite common bugs. So you can see FreeAndNil like a circuit breaker in a electric installation. If there is a short circuit (=use-after-free), then the circuit breaker will switch off (=crash the program) and with this preventing a burning house (=crooked program doing anything). You may as well run your installation without a circuit breaker and as long as there is no over current (=no use-after-free), nothing will happen. Warfly argued in his first post, that you won't need FreeAndNil for that situations, because we have better tools like HeapTrace and Valgrind. In the electric installation analogon I see them as RCD, detecting ground currents. It's a good additional measurement for detecting errors, but can't be used to replace a circuit breaker. Because FreeAndNil works in production code as well. And I argued I don't want a program crash in production code in case of a use-after-free bug, because I don't see the risk, that the house burns down because of that.

alpine

  • Hero Member
  • *****
  • Posts: 1356
Re: PSA: Don't use FreeAndNil
« Reply #43 on: May 20, 2023, 08:30:15 pm »
*snip*
I can tell you a bit. I wrote that short program for Pascal Game Contest some years ago. Unfortunately, I couldn't finished it before the deadline. That was a side scrolling 2D spaceship shooting game, written using Lazarus purely using only TCanvas, no third party graphics library.

Due to the way I code, I needed to generate a list to store all the items (rocks, bullets, ships, etc) generated on runtime and used a public variable for optimizing the performance. That variable was a pointer point to a runtime generated object. The generation of that object, reading its value and freeing it were not in the some code block, it was distributed in multiple locations, and it needed to be freed/renewed repeatedly. And that was the problem. For someone who really understands FreeAndNil command, now should understand the reason when I said ... distributed in multiple locations and repeatedly renewing.

I thought I might able to 'fix' the code, for not using FreeAndNil. But refactoring it would be too hard for me. Maybe that was a flaw in the software design. Instead of 'fixing' the code, I simply chose to use FreeAndNil, problem solved.
I see.
It is just another example of unascertained ownership. But I think it is perfectly OK as long as you know what you're doing.

@Warfley
Quote
Exceptions are just a way to extend the output space through additional control paths that carry information outside the normal return type. It's not that often used in Pascal, I think because Exceptions in Lazarus are anyoing, while in other languages exceptions that will be cought are usually not thrown in the debugger, but for example in Java it's very common, if you want to check if a file has a certain encoding, trying to decode it and if there was an exception you know that the file has not the desired encoding.
As swts , I'm also not sharing your vision for the exceptions, but that is another massive subject for discussion and clearly OOT.
"I'm sorry Dave, I'm afraid I can't do that."
—HAL 9000

Warfley

  • Hero Member
  • *****
  • Posts: 1865
Re: PSA: Don't use FreeAndNil
« Reply #44 on: May 20, 2023, 09:51:07 pm »
Now we have entered the realm of science fiction. If it takes you more than a few sentences to make a point you have no point at all! You're just fooling yourself.

I've made my statement in the first sentence of my post. Everything else are just examples:
Quote
It's not an error because it does not lead to misbehavior.
An error is your code behaving in a way that is not intended by the programmer, than the programmer concously using exceptions to archive a certain behavior is by definition not an error. If you do not agree, then you must have a different definition of what an error is.

All of the other text in my last post are just a few examples on how you can use exceptions in a way to archive a certain behavior (and in the example of STAX in a way that can only be archvied with exceptions and no other language tool that Pascal has to offer).

Let's go back to Object Pascal 101:
  • Absolutely NO exceptions in the constructor
  • Absolutely NO exceptions in the destructor
Can we agree? Thank you.
No exceptions in the constructor and destructor do behave well and can be useful to utilize. In the constructor an exception can abort the construction of the object and revert the memory allocation. Something that is not possible otherwise, unless you want to have code like this:
Code: Pascal  [Select][+][-]
  1. MyObj := TMyClass.Create;
  2. If not MyObj.WasConstructedSuccessfully then
  3.   MyObj.Free;
Not to mention on how complex the destructor is going to become when it needs to deal with partially constructed objects. Instead, in your Constructor you can just revert what you have already done (instead of having to have a partial destructor) and throw an exception to abort everything else.

And as I said, an exception in the destructor can be used to signal the user "the destructor cannot be called right now". Otherwise you would need everywhere code like this:
Code: Pascal  [Select][+][-]
  1. If MyObj.IsDestructable then
  2.   MyObj.Free;

And the problem here is that this puts the burdon on performing these checks on the callee site, which is fine for some situations, but may not be fine in others. Basically it's an opt-in mechanism. The programmer must actively choose to perform these checks. Exceptions on the other hand are an opt-out mechanism provided by the called code, that the caller must explicetly write code to ignore, otherwise it causes a crash.
So whenever you must guarantee that your class was successfully created, or you must ensure that your class is really destructable right now, you should use exceptions. As they force the user of that class to actively handle the situations where this is the case, rather than hoping that they are going to perform all the checks on callsite (of which there may be dozends if not hundreds in the final program).

As swts , I'm also not sharing your vision for the exceptions, but that is another massive subject for discussion and clearly OOT.
It's a language feature that allows for certain things that are otherwise not possible. The idea to only use Exceptions for errors, is a bit like saying "I only use if statements on integer comparisons". Sure you can, but I don't see any reason why you would artifically limit your toolset in such a way. As I said with the STAX example, exceptions are the only way to build something like that.

 

TinyPortal © 2005-2018