BestPractices: To raise, or not to raise ... an Exception in a class constructor

[Lars Fosdal 1792]

If you construct something complex that allocates memory in multiple sub-objects or blocks, how do you prevent it from leaking if an exception is raised during construction? By having exception handling in the constructors?

[dummzeuch 1505]

2 minutes ago, Lars Fosdal said:

If you construct something complex that allocates memory in multiple sub-objects or blocks, how do you prevent it from leaking if an exception is raised during construction? By having exception handling in the constructors?

No, by having a destructor that can handle partly constructed objects.

[Lajos Juhász 293]

13 minutes ago, David Heffernan said:

Again, can somebody explain what problem is caused by raising in a constructor

The only problem with it is that you've to write your code in form:

 

lObj:=TMyClass.Create(....);
try
.....
finally
  lObj.Free;
end;

Anyway you would write it in this form. So there is no problem. I believe it's just another phobia. People can be afraid from correct coding style but will happily use FreeAndNil everywhere in the code.

[Lars Fosdal 1792]

Which means that you must add exception handling in all levels of the destructor chain?

destructor TMyThing.Destroy;
begin
  try
    inherited;
  finally
    try 
      // destroy my things
    except
      // this was unfortunate
    end
  end;
end;

 

[David Heffernan 2345]

3 minutes ago, Lars Fosdal said:

Which means that you must add exception handling in all levels of the destructor chain?

No. Its pointless trying to handle exceptions in destructors. If that happens the process should terminate. 

[Dalija Prasnikar 1396]

2 minutes ago, Lars Fosdal said:

Which means that you must add exception handling in all levels of the destructor chain?

destructor TMyThing.Destroy;
begin
  try
    inherited;
  finally
    try 
      // destroy my things
    except
      // this was unfortunate
    end
  end;
end;

 

No, you don't.

 

If the instance is never constructed it will be nil in the destructor and you can just call Free on such instance. You only need to consider that any instance in the destructor can be nil, so if you are calling any other methods you need to check for nil before calling those in destructor.

 

[Lars Fosdal 1792]

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

var MyThing := TThing.Create;
try
  MyThing.DoIt;
finally
  MyThing.Free;
end;

 

[David Heffernan 2345]

5 minutes ago, Lajos Juhász said:

The only problem with it is that you've to write your code in form:

 

lObj:=TMyClass.Create(....);
try
.....
finally
  lObj.Free;
end;

Anyway you would write it in this form. So there is no problem. I believe it's just another phobia. People can be afraid from correct coding style but will happily use FreeAndNil everywhere in the code.

No. The try finally here protects the lifetime management of the instantiated object, after the constructor has completed. An exception in the constructor will bubble up from here and never enter the try. This is a very very common misunderstanding. 

[David Heffernan 2345]

Just now, Lars Fosdal said:

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

Don't you know that exceptions bubble up the call stack? If you try to handle them immediately then you've just got crappy error code type code. The entire point of exceptions is that you don't need to deal with them at the point where the error occurs. 

[Dalija Prasnikar 1396]

4 minutes ago, Lars Fosdal said:

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

var MyThing := TThing.Create;
try
  MyThing.DoIt;
finally
  MyThing.Free;
end;

 

Actual purpose of try...finally in above code is to ensure cleanup of the MyThing instance if MyThing.DoIt raises exception. If constructor raises the exception there will be automatic cleanup and MyThing.Free will never be, nor it should be called.

Edited January 12, 2022 by Dalija Prasnikar

[Lars Fosdal 1792]

So, basically

try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

[Lajos Juhász 293]

16 minutes ago, David Heffernan said:

No. The try finally here protects the lifetime management of the instantiated object, after the constructor has completed. An exception in the constructor will bubble up from here and never enter the try. This is a very very common misunderstanding. 

Exactly what I was saying.

In case you'write:

 

try

  lObj:=TMyClass.create;

finally

  lObj.Free;

end;

If the constructor raises the exception the RTL will clear that for you. However in case lObj as the name suggest is a local variable and you didn't assigned nil to it the value will be undefined and calling free on that reference can cause AV (in case you're not lucky that the value was nil).

Edited January 12, 2022 by Lajos Juhász
more information.

[David Heffernan 2345]

5 minutes ago, Lajos Juhász said:

Exactly what I was saying.

You described the correct construction pattern as a "problem" which is what confused me. 

[Dalija Prasnikar 1396]

1 hour ago, Rollo62 said:

Right, but why does it feel so bad

You tell me. It doesn't feel bad to me at all 

 

I think you are overcomplicating. 

 

If you have class that requires some setup (that will commonly not be changed once object is created) to function properly then passing those parameters through constructor and raising exception in constructor is the best and simplest thing to do.

 

Use the approach that requires the least amount of code for safely using such class and where you cannot easily forget to do something vital. Usually that will be the best code.

[Attila Kovacs 629]

 

1 hour ago, Fr0sT.Brutal said:

requires validation in every method

what if time is a factor on that calls? are you swearing while you writing the code?;)

 

[Dalija Prasnikar 1396]

21 minutes ago, Lars Fosdal said:

So, basically

try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

If you want to handle the exception on site, then the second option is the wrong way to do it for several reasons. 

 

Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so. If you are fine to bubble up that exception further on and if except part (handling the exception) cannot raise exceptions, you don't need try..finally at all.

[Attila Kovacs 629]

3 minutes ago, Dalija Prasnikar said:

Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so.

Actually I only handle exceptions where I have a plan B, otherwise I could only translate the messages to other languages.

[David Heffernan 2345]

30 minutes ago, Lars Fosdal said:

So, basically

try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

The whole point of exceptions is that in a huge number of cases it is impossible to "deal with it" at the point where the error is raised. The code has been called by another function that excepts it to succeed. If it can fail, but return normally without an exception bubbling up, then you need to return information to the caller that the function has failed. And the caller may in turn be have its own caller that needs to know this. And we are right back to exception free code where errors are indicated by boolean or integer error code return values.

 

Exceptions that bubble up the stack are only difficult to manage if you don't write your exception raising and handling code properly. For sure it is perfectly possible to write crappy exception code. But you are pointing the finger in the wrong place. 

[Dalija Prasnikar 1396]

1 minute ago, Attila Kovacs said:

Actually I only handle exceptions where I have a plan B, otherwise I could only translate the messages to other languages.

What do you mean by plan B?

[Attila Kovacs 629]

@Dalija Prasnikar Hm, I really don't know what would I do on out of memory, never had to deal with that.

[Dalija Prasnikar 1396]

2 minutes ago, Attila Kovacs said:

@Dalija Prasnikar Hm, I really don't know what would I do on out of memory, never had to deal with that.

Depends on the context.

If you open some file for processing and it turns out to be a too large so processing it raises out of memory, you just clean up and tell to the user: "Sorry, your file is too large and you don't have enough memory." But after proper cleanup your application will be in fine condition to process some other smaller file. 

On the other hand, there are situations where out of memory is not recoverable. You still may be able to show message to the user there is not enough memory, but you will just have to kill the application after that.

[Fr0sT.Brutal 900]

56 minutes ago, Attila Kovacs said:

 

what if time is a factor on that calls? are you swearing while you writing the code?;)

 

If time is a factor, these methods surely must avoid checks of init state. Yep, sometimes I'm swearing but mostly dealing with somebody's code; I'm pretty sure I'll be swearing on classes designed with your approach 😛

For me: c-tor arguments should contain crucial values that need to be set and perform some init actions so that other methods are sure the things are ready for work and they won't have to check stuff every time. You can't forget to call c-tor but you easily can forget to call separate .Init method.

[Attila Kovacs 629]

I was curious so I made this useless statistics over one of my projects

 

WSL (always backup your files before playing with scripts on them)

 

for i in `find . -name "*.pas" -type f`; do cat $i | sed -nr '/\bconstructor\b/I{:a;N;/end\;/I!ba;/begin/Ip}' | sed -e '1i\'$i'' | grep constructor; done | wc -l;

250 constructors (180 with parameter)

for i in `find . -name "*.pas" -type f`; do cat $i | sed -nr '/\bconstructor\b/I{:a;N;/end\;/I!ba;/\sraise/Ip}' | sed -e '1i\'$i'' | grep constructor; done | wc -l;

11 explicit call for raise in a constructor (with parameter)

 

if my scripts are right

 

btw. Delphi sources 5137 / 306  (3937/244 with parameter)

Edited January 12, 2022 by Attila Kovacs

[Wagner Landgraf 43]

Wow, what a long thread and discussion for something "simple". Use the 25-year old pattern for Delphi memory management:

 

Foo := TFoo.Create;
try
finally
  Foo.Free;
end;

And let the exception raising begin. Anywhere.

[Darian Miller 361]

Also consider passing items into a constructor rather than creating items within it to increase testability.