Recent

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

PascalDragon

  • Hero Member
  • *****
  • Posts: 5904
  • Compiler Developer
Re: PSA: Don't use FreeAndNil
« Reply #60 on: May 22, 2023, 10:17:18 pm »
Quote
Exceptions inside constructors and destructors are just as valid as anywhere else in Object Pascal and both the compiler and the RTL handle them correctly (except maybe FreeAndNil due to it setting the reference to Nil first).

I think you mean "Exceptions inside constructors and destructors can occur as anywhere else..." - they are certainly not valid. Get a grip - we're not going to slice our birthday cake with a screw driver just because we can. The point is to make an effort to avoid exceptions. It's simply a matter of discipline and decorum.

No, I do indeed mean valid in the sense that it is valid Object Pascal code and that both the compiler and RTL handle them correctly (as mentioned aside from FreeAndNil). Whether one would want to raise an exception inside a destructor is very questionable, but I've definitely used them in constructors especially for parameter validation (as Warfley mentioned they're the only way to abort construction of a class which should definitely be done if invalid parameters have been passed and thus the instance would be in an invalid state).

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #61 on: May 23, 2023, 03:36:11 am »
Quote
Whether one would want to raise an exception inside a destructor is very questionable...

"very questionable" and "invalid" are arguably the same thing - I think we are agreeing with each other semantically.

Quote
...abort construction of a class...

Now we're back in never never land. You don't abort the construction of a class. How do you handle such a situation in your program? I appreciate that you've taken the time to ponder the issue but I find it difficult to believe that you are considering what you're saying. It is simply best practice to avoid exceptions in your constructors and destructors.


Object Pascal allows us to write logical programs; I'm not interested in extending the output space through additional control paths.

Warfley

  • Hero Member
  • *****
  • Posts: 1870
Re: PSA: Don't use FreeAndNil
« Reply #62 on: May 23, 2023, 12:10:02 pm »
Object Pascal allows us to write logical programs; I'm not interested in extending the output space through additional control paths.

I bet you are already doing that, if you have ever used val or TryStrToInt you do exactly that. The result (i.e. the default output space) is a boolean. But through the usage of an out (or var?) Parameter this output space is extended by an integer value. But this value is only set if the boolean result is true.
I.e. you extend the outputspace from boolean to boolean + integer through a specific control flow.

So if you are truly not interested in doing this, I hope you don't use var or out parameter who's state is conditional on control flow (which I btw fully agree with, I think you should try to avoid var or out parameter whenever possible)

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #63 on: May 23, 2023, 04:48:42 pm »
My dear friend, believe me, I am tired of listening to myself talk. But foolishly I am going to make one more post although the topic has changed once more...

Returning to the question of aborting a constructor consider the tried and true boilerplate template:
Code: Pascal  [Select][+][-]
  1. ASomeObject := TSomeObject.Create;
  2. try
  3.   ..
  4. finally
  5.   ASomeObject.Free;
  6. end;

Alternatively you could follow the pattern::
Code: Pascal  [Select][+][-]
  1. try
  2.   ASomeObject := TSomeObject.Create;
  3.   try
  4.     ..
  5.   finally
  6.     ASomeObject.Free;
  7.   end;
  8. except
  9.   ..
  10. end;

The first example is a bona fide reliable approach so long as whomever wrote the class did not allow the constructor to crash. There is no hard rule anywhere that says you can't throw exceptions in your constructor but for the sake of practicality you shouldn't. (yawn) Classes should have properties to sidestep incompatible parameters. Encapsulation anybody???

And as I started doing this at a time long before Delphi introduced FreeAndNil I've never seen TryStrToInt (isn't that a .NET thing?) - I've always relied on StrToIntDef which does not incorporate var or out parameters. I agree that var or out parameters are ungainly but they've never moved me to write poetry.

Adieu.

korba812

  • Sr. Member
  • ****
  • Posts: 461
Re: PSA: Don't use FreeAndNil
« Reply #64 on: May 23, 2023, 05:41:25 pm »
A good example of using exceptions in a constructor is the TFileStream class. For me it is natural and logical.

P.S.
The title of this topic sounds a bit like:
Don't use the axe! I used it carelessly once and chopped off my foot. So don't use the axe!

PascalDragon

  • Hero Member
  • *****
  • Posts: 5904
  • Compiler Developer
Re: PSA: Don't use FreeAndNil
« Reply #65 on: May 23, 2023, 10:17:32 pm »
Quote
Whether one would want to raise an exception inside a destructor is very questionable...

"very questionable" and "invalid" are arguably the same thing - I think we are agreeing with each other semantically.

Only because I can't find a suitable usecase for having a destructor raise an exception doesn't mean that it's invalid. After all it can just as well happen that some unexpected exception (e.g. access violation, division by zero, etc.) happens during destruction. Software exceptions fall right into the same bucket.

My dear friend, believe me, I am tired of listening to myself talk. But foolishly I am going to make one more post although the topic has changed once more...

Returning to the question of aborting a constructor consider the tried and true boilerplate template:
Code: Pascal  [Select][+][-]
  1. ASomeObject := TSomeObject.Create;
  2. try
  3.   ..
  4. finally
  5.   ASomeObject.Free;
  6. end;

Alternatively you could follow the pattern::
Code: Pascal  [Select][+][-]
  1. try
  2.   ASomeObject := TSomeObject.Create;
  3.   try
  4.     ..
  5.   finally
  6.     ASomeObject.Free;
  7.   end;
  8. except
  9.   ..
  10. end;

The first example is a bona fide reliable approach so long as whomever wrote the class did not allow the constructor to crash. There is no hard rule anywhere that says you can't throw exceptions in your constructor but for the sake of practicality you shouldn't. (yawn) Classes should have properties to sidestep incompatible parameters. Encapsulation anybody???

And there can just as well be classes that don't make sense to be constructed if the parameters aren't valid. Take korba812's example of a TFileStream: if the file can't be opened it makes no sense to return a valid class instance. Another example is TThread: if the thread can't be created (e.g. if there's not enough memory to allocate the stack or you're not allowed to create more threads due to some ulimit) then it also makes no sense to have the constructor return a valid class instance.

And yes, in case you don't want such an exception to bubble up your call stack you have to use an additional tryexcept block then.

If the constructor wouldn't abort with an exception then you'd either get the exception when calling a method due to the state of the class not being valid (which would return in a tryexcept block anyway) or each method would have to return some error checking result the checking of which would have to be checked which would clutter the code much more than the tryexcept block.

And as I started doing this at a time long before Delphi introduced FreeAndNil I've never seen TryStrToInt (isn't that a .NET thing?) - I've always relied on StrToIntDef which does not incorporate var or out parameters. I agree that var or out parameters are ungainly but they've never moved me to write poetry.

No, TryStrToInt is not a .NET thing. It was part of Delphi long before .NET even existed. TryStrToInt enables the user to directly decide whether the input was valid while with StrToIntDef you either need to pick a default value that isn't valid for your use case or you need to use constructs like the following:

Code: Pascal  [Select][+][-]
  1. v := StrToIntDef(s, 0);
  2. if (v = 0) and (s <> '') then
  3.   Writeln('Got error')
  4. else
  5.   Writeln('Got valid value');

With TryStrToInt it's simply:

Code: Pascal  [Select][+][-]
  1. if TryStrToInt(s, v) then
  2.   Writeln('Got valid value')
  3. else
  4.   Writeln('Got error');

SymbolicFrank

  • Hero Member
  • *****
  • Posts: 1315
Re: PSA: Don't use FreeAndNil
« Reply #66 on: May 24, 2023, 10:05:55 am »
I agree with kupferstecher, that the last thing you want your application to do after release, is crash. Because, at that moment, all bets are off and there is nothing more to be done.

From the start of programming, people have tried to make a programming language that allows you to check if your program is free of bugs. The problems with this approach are, that you cannot track (most) logical bugs, and that most applications aren't written in one go, by one programmer. Perhaps small ones, written in assembly, that run on simple microcontrollers, but even those tend to contain firmware, ie: third-party software over which you have no control.

If you want your application to crash if a mistake is made, you have to limit that to the internal state, which is mostly useless. Because programs process data, so part of the state machine is the validity of the input data. If you always assume that your inputs (parameters and data) are "valid", you will have a buggy program. It might work with your sanitized test-data, but as soon as it gets released, crap will go in.

So, the best way to handle it is programming defensively: never assume any state or value not controlled inside the function. And even then: if that's a class or uses an external library to create or execute functions, make sure you check the result. And if things go wrong, try to save whatever you can.

That's also why I don't like exceptions.

Warfley

  • Hero Member
  • *****
  • Posts: 1870
Re: PSA: Don't use FreeAndNil
« Reply #67 on: May 24, 2023, 10:28:20 am »
Yes, limiting the state space is exactly what you want.

For example let's say you need an email address from the user. One way to do it is to create an email wrapper type for string (using advanced records) which on initialization checks if the string is a valid email. So once initialized, you only pass that email class around, and fpc typechecking makes sure you cannot mix it up with non email strings. This way you have limited your program space from all possible strings to only valid email addresses, and can be sure whenever you have a function that takes the email address that it was previously checked to be valid.

Reducing the state space is really important, because it reduces the mental load on knowing when to do what checks (as you only check once which reduces the state space for the later run of the program), and you can use automatic tools, like compiler typechecks, to help you enforce this

When processing data, you should validate it always as early as possible, and then when passing it around you know that it is valid and have a small state space. Then you can use unit tests to extensively test your functions which is only feasible if you are operating in an reduced state space (because an exhaustive test for arbitrary inputs is kind of impossible to make).
This way you have a broad input space on the outer layers, but only a very small one in you application core, which is where most if not all of your logic resides, and where bugs most matter
« Last Edit: May 24, 2023, 10:47:24 am by Warfley »

SymbolicFrank

  • Hero Member
  • *****
  • Posts: 1315
Re: PSA: Don't use FreeAndNil
« Reply #68 on: May 24, 2023, 11:47:00 am »
Well, yes, of course. But you just assumed, that all internal data is sanitized and valid. That the internal state is always within the design specifications. To do that, you cannot have parts that aren't written by you or with a different set of specs. And not only does everyone interpret those in a different way (even the future you), but they change.

Take that email address as an example: in the past, you could check the syntax. It had to end in a limited set of 2- or 3-letter top-level domains, the domain name itself could be checked with a DNS lookup (although it is rare to do that) and all the individual chars could only consist of a limited subset of ASCII. But things have changed. Nowadays new top-level domains are introduced at a fast speed and often any Unicode symbols are allowed for the other chars. The only valid thing left to check is, if there is (at least) one '@' in the email address. That's it.

So, if your code handles email and/or email addresses, be prepared for many fail states.
« Last Edit: May 24, 2023, 11:49:32 am by SymbolicFrank »

alpine

  • Hero Member
  • *****
  • Posts: 1372
Re: PSA: Don't use FreeAndNil
« Reply #69 on: May 24, 2023, 01:49:01 pm »
Yes, limiting the state space is exactly what you want.
Please, define "state space". Is it, in a case of function, its "domain" and/or "co-domain"?

A word of practicality:
 
Suppose you're in a hunt for a bug and you have wrote something like:
Code: Pascal  [Select][+][-]
  1.   try
  2.     MyParam := StrToInt(StrValue);
  3.   except
  4.     MyParam := MyParamDefault;
  5.   end;

And you're perfectly sure that it works as it should and it is not connected to what you're looking for.

Then you run in a debugging session and start acting the scenario that leads to the manifestation of the bug, waiting for debugger to stop on a breakpoint or exception. Then suddenly the debugger stops into except clause of the above, pausing everything and causing all kind of additional exceptions about timeouts and the like. Your test scenario play is ruined and you must start from the beginning. That is sometimes not cheap.

Of course, you can put EConvertError into ignored exceptions list, but that will ignore it everywhere. You'll miss the chance to catch it in places where you don't expect it. Additionally, you must consider all such places where you expect exceptions not as real exceptions but as something usual and put them into the ignore list in advance.

Instead, written like
Code: Pascal  [Select][+][-]
  1.   MyParam := StrToIntDef(StrValue, MyParamDefault);
or even
Code: Pascal  [Select][+][-]
  1.    if not TryStrToInt(StrValue, MyParam) then
  2.     MyParam := MyParamDefault; // you can put a breakpoint here if you need to
It won't cause any trouble at all.

BTW, isn't it raising an exception an extension of the co-domain (if you consider it "the output space"), i.e. StrToInt: [Strings domain] --> [Longint domain+EConvertError]

To summarize: Don't use (abuse) exceptions for things they're not meant for.
"I'm sorry Dave, I'm afraid I can't do that."
—HAL 9000

Swami With The Salami

  • Guest
Re: PSA: Don't use FreeAndNil
« Reply #70 on: May 24, 2023, 05:09:50 pm »
Quote
If the constructor wouldn't abort with an exception then you'd either get the exception when calling a method due to the state of the class not being valid...or each method would have to return some error checking...

Yes, if I were to code TFileStream the constructor would not crash. Subsequent calls would.

Programmers should be able to confidently instantiate a class and free it without wrapping their code in additional try .. except blocks. Object Pascal doesn't force us to provide such confidence but ignoring it is anathemic in my opinion. It isn't difficult to understand what I mean by valid but you do not appear to be willing to stand under (support) it.

But that is acceptable. As we know a programmer's confidence if too often false confidence:

Code: Pascal  [Select][+][-]
  1. v := StrToIntDef(s, 0);
  2. if (v = 0) and (s <> '') then                // should be (s <> '0')
  3.   Writeln('Got error')
  4. else
  5.   Writeln('Got valid value');

Admittedly my insistence is thoroughly annoying. Sorry.

Warfley

  • Hero Member
  • *****
  • Posts: 1870
Re: PSA: Don't use FreeAndNil
« Reply #71 on: May 24, 2023, 06:51:13 pm »
Then you run in a debugging session and start acting the scenario that leads to the manifestation of the bug, waiting for debugger to stop on a breakpoint or exception. Then suddenly the debugger stops into except clause of the above, pausing everything and causing all kind of additional exceptions about timeouts and the like. Your test scenario play is ruined and you must start from the beginning. That is sometimes not cheap.

Of course, you can put EConvertError into ignored exceptions list, but that will ignore it everywhere. You'll miss the chance to catch it in places where you don't expect it. Additionally, you must consider all such places where you expect exceptions not as real exceptions but as something usual and put them into the ignore list in advance.
Yes, that's sadly a disadvantage of Lazarus. Tools for other languages are far more advanced. The java debugger for example allows you to disable halting on exceptions that are handled by a try except, and even allow you to enable or disable certain exceptions by their line in the code. The same is with .Net or python, or even most C++ IDEs with gdb.

But you can't fault a language feature for the tooling bring a bit behind. Also it's a chicken and egg problem. People rarely use exceptions, which is why they aren't supported so we'll in debugging with Lazarus, which you now use to argue that you shouldn't use exceptions.

Not using a feature because the tooling is bad is a self fulfilling prophecy. If no one uses the feature there is no demand to support it better

Also you don't just halt execution on an exception, you always do so on breakpoints. If your program cannot be halted by the debugger, either for exceptions or for breakpoints, you built yourself an undebuggable program, and then you have much more problems than just that one exception

To summarize: Don't use (abuse) exceptions for things they're not meant for.

To summarize, we just need better tools.
« Last Edit: May 24, 2023, 06:59:32 pm by Warfley »

alpine

  • Hero Member
  • *****
  • Posts: 1372
Re: PSA: Don't use FreeAndNil
« Reply #72 on: May 24, 2023, 07:56:54 pm »
But you can't fault a language feature for the tooling bring a bit behind. Also it's a chicken and egg problem. People rarely use exceptions, which is why they aren't supported so we'll in debugging with Lazarus, which you now use to argue that you shouldn't use exceptions.
That's not the main argument. Just another big inconvenience. Even with advanced tooling and writing as you propose, you must spend much of your time on preparation instead of a real hunt. You can simply avoid that.

Also you don't just halt execution on an exception, you always do so on breakpoints. If your program cannot be halted by the debugger, either for exceptions or for breakpoints, you built yourself an undebuggable program, and then you have much more problems than just that one exception
C'mon, you know what I'm talking about - there is no real world program that tolerate arbitrary delays. It always have to communicate with a front/back-end or to have background threads - all around exists timeouts and timings.

And once again:
Quote
BTW, isn't it raising an exception an extension of the co-domain (if you consider it "the output space"), i.e. StrToInt: [Strings domain] --> [Longint domain+EConvertError]
as opposed to:
TryStrToInt:  [Strings domain] --> [Longint domain+false]
"I'm sorry Dave, I'm afraid I can't do that."
—HAL 9000

Warfley

  • Hero Member
  • *****
  • Posts: 1870
Re: PSA: Don't use FreeAndNil
« Reply #73 on: May 24, 2023, 10:07:10 pm »
C'mon, you know what I'm talking about - there is no real world program that tolerate arbitrary delays. It always have to communicate with a front/back-end or to have background threads - all around exists timeouts and timings.
Well I try to avoid threads as much as possible (as they usually just make more problems than they solve), but of course you sometimes have some form of networking happening in the background. But for those I usually have a debugging configuration where I disable timeouts, or use mock servers/clients to just emulate server behavior, to not have to deal with timeouts during debugging. Because even with no exceptions, I want to be able to step through the program without completely destroying the execution (again, unit tests. Testing on the production code is not feasible as soon as the program reaches a certain complexity. And there you can abstract all those real world interactions away).

And once again:
Quote
BTW, isn't it raising an exception an extension of the co-domain (if you consider it "the output space"), i.e. StrToInt: [Strings domain] --> [Longint domain+EConvertError]
as opposed to:
TryStrToInt:  [Strings domain] --> [Longint domain+false]
Yes, as I have written in an much earlier post, in functional languages, which do not have this typical top to bottom control flow, this is literally how exceptions are implemented, usually through some form of except monad. E.g. in Haskell the file open function looks like this
Code: Text  [Select][+][-]
  1. open :: FilePath -> ExceptionalT IOException IO Handle

In an imperative language like pascal it's a bit more complicated because it not just adds to the output space of the function that raises the exception, but also to every function that calls said function and does not catch it. In Java you need to annotate this also in every function on the stack.

So basically every function that can raise an exception amends the output space. But the thing is, compared e.g. to TryStrToInt, you can ignore the result, in fact you need to actively write code to test if it was successful, this is opt-in handling, where if you do nothing you ignore it. Exceptions on the other hand will, if not handled crash your program. So if you want to ignore them you need to actively write code to catch them. So they are an opt-out way

And this is the biggest difference. When you have something the user needs to react to (e.g. a network stream closes), then you should use the exception, because the user can't easily ignore this. If you have a function that the user should be able to easily ignore (like if the logging file cannot be opened, and logging is not essential to the application).

Exceptions are good if you need the user to react, return types are good when you can savely ignore it

SymbolicFrank

  • Hero Member
  • *****
  • Posts: 1315
Re: PSA: Don't use FreeAndNil
« Reply #74 on: May 24, 2023, 11:04:58 pm »
I got one of my libraries reviewed by a Java/C# programmer once, who scored it 0/10.

"If something goes wrong, you should always throw an exception and let it crash. Because if you don't, nobody will be forced to fix it, so they won't. And in your code, all the bugs are covered up, which is the worst thing you can do."

I also know quite a few managers who think like that. The problem being, that with that mindset, the goal is to never finish the project and always turn it into an unlimited SLA. Because the frequent crashing prevents releasing it. Bugs are good, more bugs are better. Lock-in. And it produces more money that way in the short term, until the customers get fed up with it and the company goes broke. But the shareholders love it.

 

TinyPortal © 2005-2018